Improve span conversion tests #74387
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
jjonescz
force-pushed
the
FirstClassSpan-11-NetTests
branch
from
0cf46ef
to
c899a80
Compare
cston
reviewed
Jul 15, 2024
| var comp = CreateCompilationWithSpan(source, parseOptions: TestOptions.Regular.WithLanguageVersion(langVersion)); | ||
| var comp = CreateCompilationWithSpanAndMemoryExtensions(source, | ||
| parseOptions: TestOptions.Regular.WithLanguageVersion(langVersion), | ||
| noStringToReadOnlySpanConversionInSource: true); |
There was a problem hiding this comment.
There was a problem hiding this comment.
Is this the only place
noStringToReadOnlySpanConversionInSourceis used? If so, could we just special-case this use?
Yes, you're right, I will add only what's needed from the span source into the test and revert the complicated workaround, thanks.
And is this a scenario where we expect to support the non-standard definition?
Yes, the non-standard user-defined operator from string to ROS<T> is expected to be supported here - because we do not ignore UDCs from null to ROS<T> (see #74002 (comment)), so the UDC is used.
This reverts commit 2e98bd2.
cston
approved these changes
Jul 16, 2024
This was referenced Oct 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Follow up on #74232 (comment), i.e., switching to
CreateCompilationWithSpanAndMemoryExtensionsso Span from the .NET runtime library is used in tests when not running on .NET Framework. The rest are modifications needed because of that.I'm doing these modifications before merging main into the feature branch so that the merge PR doesn't need to contain any non-trivial changes like this. (Some changes would be needed even if we didn't switch to
CreateCompilationWithSpanAndMemoryExtensionsbecause of the Span sources consolidation in #74281.)Commit-by-commit review might be useful. (EDIT: Skip the fourth one 2e98bd2 as it's been reverted.)
Test plan: #73445