-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sockets #101
Sockets #101
Conversation
@@ -193,3 +175,5 @@ public int GetResult() | |||
} | |||
} | |||
} | |||
|
|||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line at the end of file.
@@ -2,147 +2,129 @@ | |||
// The .NET Foundation licenses this file to you under the MIT license. | |||
// See the LICENSE file in the project root for more information. | |||
|
|||
#if NETCOREAPP2_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead control the files to be built at the csproj level? I think that having a single point would be cleaner. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we instead control the files to be built at the csproj level? I think that having a single point would be cleaner. Thoughts?
Very good point!
I was thinking about this "problem" and I think that controlling it on .csproj level would be better. We do it already for some other benchmarks.
However I am not sure what we should do:
Specify all the excluded files in explicit way per platform?
Example:
<ItemGroup Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">
<Compile Remove="coreclr\Math\Functions\Single\**" />
</ItemGroup>
Or maybe introduce some patterns in file names?
Sth like;
Class.Core.cs
Class.Core21+.cs
Example:
<ItemGroup Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">
<Compile Remove="**\*Core*" />
</ItemGroup>
@jorive what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this pattern is easier to control/monitor:
<ItemGroup Condition=" '$(TargetFrameworkIdentifier)' == '.NETFramework' ">
<Compile Remove="coreclr\Math\Functions\Single\**" />
</ItemGroup>
@DrewScoggins @adiaaida @brianrob What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that controlling the includes in the csproj is much easier to reason about. Rather than using Remove, I'd recommend using <Compile Include="path/to/file.cs" />
. This will also work better when we enable things for .NET Framework, which does not use the Core behavior of including all cs files by default. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also work better when we enable things for .NET Framework, which does not use the Core behavior of including all cs files by default.
It's dependent on what .csproj
are you using, not which framework you target. So if we have a new "sdk style" csproj targeting .NET 4.6 it's going to include all .cs
files by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jorive. I think that switching on the target framework and removing the pieces that we don't want makes the most sense, but I think that we should make sure to call it out in both comments in both the .csproj file as well as the source of test. Make it very clear where these tests will and will not run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jorive I removed the #ifdefine
blocks, added the condition to .csproj
and added the comments to the code as @DrewScoggins suggested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes #61
Again, I was unable to run this benchmark with xunit-performance so only BDN results below.