Move #if NET directives forward
#75500
Conversation
dotnet-issue-labeler
bot
added
the
untriaged
| @@ -117,7 +117,7 @@ public static bool TryHash(ReadOnlySpan<byte> source, Span<byte> destination, ou | |||
| return false; | |||
| } | |||
|
|
|||
| #if NET7_0_OR_GREATER | |||
| #if NET8_0_OR_GREATER | |||
There was a problem hiding this comment.
i don't understaand this change. it wraps using Uint128. Which was added in .net 7.
There was a problem hiding this comment.
From #75453 (comment):
When people see
#if NET7in code but nonet7.0in the TFM list then there is a tendency to ask "wait, is that stale? can we remove that?" Hence I like to roll them forward to make sure that we still need them and to make it clearer to readers that they are intended.
There was a problem hiding this comment.
Correct. Essentially we do not target net7.0 anymore hence leaving #if NET7_... directives around opens questions when reading the code. At a glance it's unclear if that is a directive that we still need or one that was accidentally left after other changes.
When doing this changes in the past I've found all manner of unused / legacy directives in the code (many I added to be fair). Found that if we keep pushing it forward like this it makes it easier to see that it's tied to our modern build semantics
There was a problem hiding this comment.
so in this case, we want to move to NET8 pragma, even though it's really that it was NET7 that added this api? if that's the design, then that's fine with me. I just wanted to confirm that was how we think it's best to do things.
| @@ -342,7 +342,7 @@ public static void CopyAccumulators(ref State state, ulong* accumulators) | |||
| { | |||
| fixed (ulong* stateAccumulators = state.Accumulators) | |||
| { | |||
| #if NET7_0_OR_GREATER | |||
| #if NET8_0_OR_GREATER | |||
There was a problem hiding this comment.
same with these checks. the apis we're gating around are on .net7. this would mean we would fall back to the lower performance code there.
There was a problem hiding this comment.
the apis we're gating around are on .net7. this would mean we would fall back to the lower performance code there.
We no longer target net7.
| @@ -19,7 +19,7 @@ internal static class TextReaderExtensions | |||
| // If we're on .NET Core 7.0 the implementation is easy, but on older versions we don't have the helper and since we also don't have | |||
| // Microsoft.VisualStudio.Threading's WithCancellation helper we have to inline the same approach. | |||
|
|
|||
| #if NET7_0_OR_GREATER | |||
There was a problem hiding this comment.
Comment also needs updating.
There was a problem hiding this comment.
Although it's not clear to me why this isn't just #if NET now...
There was a problem hiding this comment.
Although it's not clear to me why this isn't just
#if NETnow...
Sounds like a pre-existing issue. This PR should be just mechanical update - when we update our TFM, we move these #ifs forward.
src/Workspaces/Core/MSBuild.BuildHost/Rpc/Contracts/TextReaderExtensions.cs
Outdated
Show resolved
Hide resolved
Part of #75453.
cc @jaredpar