-
Notifications
You must be signed in to change notification settings - Fork 344
Remove unnecessary cast now that we are targeting C# 7.3 with latest compiler #2299
Conversation
FYI, @MisinformedDNA, @JeremyKuhne https://ci.dot.net/job/dotnet_corefxlab/job/master/job/ubuntu16.04_release_prtest/1979/console
|
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 the .props change is fine, but I'd like somebody from the compiler team to chime in just to ensure that compiling in this manner from the command line is supported.
@agocke - does this change look good to you?
@dotnet/dnceng, @maririos - is it possible for us to get the repro tool working in corefxlab as well? We have intermittent test failures here that are hard to reproduce and debug (especially in this instance since we can't tell the name of the failing test). |
@@ -15,7 +15,5 @@ | |||
<AssemblyOriginatorKeyFile>$(MSBuildThisFileDirectory)Key.snk</AssemblyOriginatorKeyFile> | |||
<LangVersion>latest</LangVersion> | |||
</PropertyGroup> | |||
<ItemGroup Condition="'$(BuildingInsideVisualStudio)' != 'true'"> | |||
<PackageReference Include="Microsoft.NETCore.Compilers" Version="$(RoslynVersion)" PrivateAssets="All" /> |
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.
Why don't you need this anymore? Do you have a min req for .NET Core that is guaranteed to have C# 7.3?
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.
Do you have a min req for .NET Core that is guaranteed to have C# 7.3?
Yes we do, but it looks like the csc.dll within roslyn that comes with the dotnet cli that we install has a recent enough version and has support for C#7.3.(https://dotnet.myget.org/feed/roslyn/package/nuget/Microsoft.NETCore.Compilers/2.8.1-beta6-62911-06)
Otherwise, the code change in this PR to remove the cast wouldn't compile (based on recent overload resolution changes - dotnet/roslyn#24756). That change wouldn't build on C#7.2.
System\Buffers\Writer\BufferWriter_writable.cs(14,16): error CS0306: The type 'Span' may not be used as a type argument [D:\GitHub\Fork\corefxlab\src\System.Buffers.ReaderWriter\System.Buffers.ReaderWriter.csproj]
Leftover change from #2161