Address feedback from previous pattern matching ReadOnlySpan<char> change #59451
Conversation
cston
force-pushed
the
44388-comments
branch
from
aee1274
to
9dcae80
Compare
cston
force-pushed
the
44388-comments
branch
from
9dcae80
to
f0988af
Compare
src/Compilers/CSharp/Portable/Compiler/MethodBodySynthesizer.Lowered.cs
Outdated
Show resolved
Hide resolved
| static void Main() | ||
| { | ||
| var s = new ReadOnlySpan<char>(new char[0]); | ||
| _ = s is ""str""; |
There was a problem hiding this comment.
I'm fine with this scenario not working, but curious if there was a specific reason we can't compile this when 'Length' is missing? Seems like we only actually need to call MemoryExtensions.SequenceEqual? #ByDesign
There was a problem hiding this comment.
You're right, it looks like we're using Span<char>.Length or ReadOnlySpan<char>.Length for lowering switch arms that use "" (in CodeGenerator.EmitStringSwitchJumpTable()), but not for lowering is "".
That said, I think I'd rather ensure Length exists for both scenarios.
| // (5,57): error CS0037: Cannot convert null to 'ReadOnlySpan<char>' because it is a non-nullable value type | ||
| CreateCompilation(source, targetFramework: TargetFramework.NetCoreApp, references: new[] { Net50.SystemMemory }, parseOptions: TestOptions.RegularPreview) | ||
| .VerifyEmitDiagnostics( | ||
| // (5,57): error CS0150: A constant value is expected |
There was a problem hiding this comment.
It feels like the previous diagnostic was easier to understand. #ByDesign
There was a problem hiding this comment.
It feels like the previous diagnostic was easier to understand.
Agreed. To be clear though, the diagnostic changed because the ReadOnlySpan<T> type declaration in NetCoreApp has different conversion operators than the TestSources declaration we were using previously.
I'll leave this as is.
| IL_0005: call ""System.ReadOnlySpan<char> string.op_Implicit(string)"" | ||
| IL_000a: stloc.0 | ||
| IL_000b: ldloca.s V_0 | ||
| IL_000d: call ""int System.ReadOnlySpan<char>.Length.get"" |
There was a problem hiding this comment.
The spec doesn't mention any usage of Length.
What is the purpose? MemoryExtensions.SequenceEqual already compares lengths before comparing bits. #ByDesign
There was a problem hiding this comment.
This aligns with the behavior for switch with a string value (see sharplab.io).
Let me know if you'd prefer to use SequenceEqual() in all cases.
There was a problem hiding this comment.
Ah, that makes sense. Thanks.
nit: The spec should probably mention our usage of those two Length properties.
| @@ -149,7 +149,7 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState, | |||
| LabelSymbol start = F.GenerateLabel("start"); | |||
|
|
|||
|
|
|||
| // This method should be kept consistent with SynthesizedStringSwitchHashMethod.ConstructStringHash | |||
| // This method should be kept consistent with SynthesizedStringSwitchHashMethod.ComputeStringHash | |||
There was a problem hiding this comment.
nit: extra empty line above #Resolved
Is this spec'ed? Consider adding xml doc on this type. In reply to: 1039970680 Refers to: src/Compilers/CSharp/Portable/Compiler/MethodBodySynthesizer.Lowered.cs:129 in 27f2ff8. [](commit_id = 27f2ff8, deletion_comment = False) |
| IL_0014: ldloc.0 | ||
| IL_0015: ldstr ""string 1"" | ||
| IL_001a: call ""System.ReadOnlySpan<char> System.MemoryExtensions.AsSpan(string)"" | ||
| IL_001f: call ""bool System.MemoryExtensions.SequenceEqual<char>(System.ReadOnlySpan<char>, System.ReadOnlySpan<char>)"" |
There was a problem hiding this comment.
nit: Might it be worthwhile to produce a private implementation help that would do AsSpan and SequenceEqual, so that we don't repeat it in every arm? #ByDesign
There was a problem hiding this comment.
Will leave as is since that would only shrink each call site by one method call.
There was a problem hiding this comment.
Don't we expect multiple call sites though?
There was a problem hiding this comment.
Yes, this would shrink each call site by one method call. It's not clear how that might affect perf though, particularly with respect to inlining. As we discussed offline, let's leave this calling pattern as is and address any specific perf issues as issues are identified.
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
Addresses comments marked pending in #44388.
Proposal: pattern-match-span-of-char-on-string.md
Test plan: #59191