Skip to content

Additional tests for pattern matching on Span<char>#59876

Merged
RikkiGibson merged 5 commits intodotnet:features/patterns-span-charfrom
cston:more-pattern-span-tests
Mar 4, 2022
Merged

Additional tests for pattern matching on Span<char>#59876
RikkiGibson merged 5 commits intodotnet:features/patterns-span-charfrom
cston:more-pattern-span-tests

Conversation

@cston
Copy link
Copy Markdown
Contributor

@cston cston commented Mar 2, 2022

Test plan #59191

@ghost ghost added the Area-Compilers label Mar 2, 2022
@cston cston force-pushed the more-pattern-span-tests branch from c8653ee to fc5cd4f Compare March 2, 2022 19:10
@cston cston force-pushed the more-pattern-span-tests branch from fc5cd4f to fea2e4c Compare March 2, 2022 19:19
@cston cston marked this pull request as ready for review March 2, 2022 19:21
@cston cston requested a review from a team as a code owner March 2, 2022 19:21
@cston cston requested review from a team and removed request for a team March 2, 2022 19:26
";
CreateCompilationWithSpan(source, parseOptions: TestOptions.RegularPreview)
.VerifyEmitDiagnostics(
.VerifyDiagnostics(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VerifyDiagnostics

Most of these cases were changed from VerifyDiagnostics in the previous PR. This first commit changes those cases back, to ensure the errors are reported before emit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests for missing members use VerifyEmitDiagnostics() since missing members may affect the synthesized string hash methods.

}

constantValue = expression.ConstantValue;
convertedExpression = BindToNaturalType(expression, diagnostics);
Copy link
Copy Markdown
Member

@jcouv jcouv Mar 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, what scenario was affected? #Resolved

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were a couple of cases that were affected: matching against a constant interpolated string and matching against a switch expression I believe.

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 3) with some follow-up discussion about VerifyDiagnostics

@jcouv jcouv self-assigned this Mar 3, 2022
@cston cston force-pushed the more-pattern-span-tests branch from 4086943 to b86269e Compare March 3, 2022 04:32

/// <summary>
/// DecisionDagRewriter.EnsureStringHashFunction() does not generate
/// a hash function if the span indexer is missing.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should DecisionDagRewriter.EnsureStringHashFunction() report an error if the indexer is missing rather than silently falling back to the non-hash implementation?

Copy link
Copy Markdown
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks (iteration 5)

@RikkiGibson
Copy link
Copy Markdown
Member

Once again, we don't think the rebuild failures are caused by this PR.

@RikkiGibson RikkiGibson merged commit 849ec84 into dotnet:features/patterns-span-char Mar 4, 2022
@cston cston deleted the more-pattern-span-tests branch March 4, 2022 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants