Skip to content

Conversation

@AlekseyTs
Copy link
Contributor

Also ensure SymbolDisplay for VB can handle symbols for VB checked built-in operators after the recent change in #63604. This fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1615080.

…perators for the target language.

Also ensure SymbolDisplay for VB can handle symbols for VB checked built-in operators after the
recent change in dotnet#63604. This fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1615080.
@AlekseyTs
Copy link
Contributor Author

@333fred, @dotnet/roslyn-compiler Please review

if (sourceUserDefinedOperatorSymbolBase is SourceUserDefinedConversionSymbol)
{
addUserDefinedConversionName(symbol, operatorName);
addUserDefinedConversionName(symbol, tryGetUserDefinedConversionTokenKind(operatorName), operatorName);
Copy link
Member

@jaredpar jaredpar Sep 19, 2022

Choose a reason for hiding this comment

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

There is an implicit dependency here that tryGetUserDefinedConversionTokenKind does not return SyntaxKind.None. Why is it safe to assume that on this code path but other code paths explicitly handle its return? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is it safe to assume that on this code path but other code paths explicitly handle its return?

This code path handles C# symbols (see is SourceUserDefinedOperatorSymbolBase check above, for those the helper is expected to always succeed.

@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler For the second review.

1 similar comment
@AlekseyTs
Copy link
Contributor Author

@dotnet/roslyn-compiler For the second review.

@AlekseyTs AlekseyTs merged commit dd04453 into dotnet:main Sep 21, 2022
@ghost ghost added this to the Next milestone Sep 21, 2022
@Cosifne Cosifne modified the milestones: Next, 17.4 P3 Sep 26, 2022
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