Use IndexOfAnyValues in Regex.Escape #78667
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-regularexpressions Issue DetailsContributes to #78204
|
| @@ -2081,6 +2076,27 @@ private bool IsCaptureSlot(int i) | |||
| // ' a b c d e f g h i j k l m n o p q r s t u v w x y z { | } ~ | |||
| 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, Q, S, 0, 0, 0}; | |||
|
|
|||
| #if NET7_0_OR_GREATER | |||
There was a problem hiding this comment.
I merged the PR, but... this should be NET8_0_OR_GREATER. Seems like an artifact of our build system right now that this work. Once we fix that, these are going to start to fail presumably.
cc: @ViktorHofer
There was a problem hiding this comment.
Right, the NET8 directives don't seem to exist yet.
Once they are added, would we ever build this with an actual 7.0 target? That is, would this continue to just "work", while being misleading to the reader since the API doesn't actually exist on 7?
There was a problem hiding this comment.
It might. We have several libraries that build for older .NET Core versions, and while System.Text.RegularExpressions.dll itself isn't one of them, it's feasible the source generator could.
Plus, it's confusing.
There was a problem hiding this comment.
Personally, I would have used #if NET here instead as presumably at the time that we would ship a .NETCoreApp version of the Regex source generator (which isn't planned), we wouldn't build older tfms like net6.0 and net7.0 anymore.
Can you please either submit a PR to change this to #NET or create an issue that tracks updating this to NET8_0_OR_GREATER?
Updating the tfm to net8.0 is being take care of via #78354.
There was a problem hiding this comment.
Personally, I would have used #if NET here
FWIW I think that's just as if not more confusing. All of this is .NET.
There was a problem hiding this comment.
Interesting that you object to that. Can you please elaborate? We use #if NETCOREAPP all over the places here in the repo to differentiate between "modern .NET -> NETCOREAPP" and older frameworks like ".NET Standard" and ".NET Framework".
There was a problem hiding this comment.
Both .NET 7 and .NET 8 are netcoreapp.
There was a problem hiding this comment.
That's why I suggested to use #if NET as if we would ever make the source generator target NETCOREAPP we likely wouldn't build net7.0 anymore. We don't version preprocessor directives in the BCL if there isn't a current need. I.e., there aren't any NETCOREAPP_3_1_OR_GREATER (or earlier) symbols because we don't build such frameworks anymore.
There was a problem hiding this comment.
.NET Core 3.1 is old and unsupported. We just shipped NET 7 last week. I'm glad it's not confusing to you; it's highly confusing to me. Many projects in the repo build for .NET 7, so seeing code that doesn't actually work on .NET 7 ifdef'd in a way that suggests it should is wrong, IMHO.
ghost
locked as resolved and limited conversation to collaborators
Contributes to #78204