Fix SyntaxGenerator for checked operators #63411
Conversation
ghost
added
the
Community
src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs
Outdated
Show resolved
Hide resolved
|
@CyrusNajmabadi This is ready for review. |
src/Workspaces/CSharp/Portable/CodeGeneration/CSharpSyntaxGenerator.cs
Outdated
Show resolved
Hide resolved
| IEnumerable<SyntaxNode>? statements = null) | ||
| { | ||
| throw new NotImplementedException(); | ||
| } |
There was a problem hiding this comment.
@CyrusNajmabadi A new method that allows me to pass isChecked. I couldn't think of another good way.
There was a problem hiding this comment.
that makes sense. w hy is this virtual though and not abstract?
There was a problem hiding this comment.
I incorrectly assumed the type is subclass-able when I saw this:
roslyn/src/Workspaces/Core/Portable/Editing/SyntaxGenerator.cs
Lines 194 to 203 in 8f96b1b
Given there are already non-public abstract methods, it's indeed okay to make the new methods abstract.
This is fixed in the last commit now.
Co-authored-by: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
| WellKnownMemberNames.UnaryPlusOperatorName => OperatorKind.UnaryPlus, | ||
| _ => throw new ArgumentException("Unknown operator kind."), | ||
| }; | ||
| private protected virtual int GetOperatorSyntaxKind(IMethodSymbol method, out bool isChecked) |
There was a problem hiding this comment.
why virtual and not abstract (applies to all methods like this).
There was a problem hiding this comment.
@CyrusNajmabadi SyntaxGenerator is public. Adding new abstract members is breaking.
There was a problem hiding this comment.
SyntaxGenerator has internal-abstracts. It cannot be publicly subclassed :) you're fine adding a new abstract to it.
|
@CyrusNajmabadi This is ready for another look. |
| @@ -202,18 +202,36 @@ internal SyntaxNode MethodDeclaration(IMethodSymbol method, string name, IEnumer | |||
| throw new NotImplementedException(); | |||
| } | |||
|
|
|||
| private protected abstract SyntaxNode OperatorDeclaration( | |||
| int syntaxKind, | |||
There was a problem hiding this comment.
i think i'd prefer just passing hte operator name along. thsi also includes 'checked' so it can subsume the first two parameters.
|
@CyrusNajmabadi Can this be merged? |
|
@CyrusNajmabadi Can you take a look please? Thanks! |
|
Thanks @Youssef1313 :) |
Fixes #63410
@CyrusNajmabadi What do you think of the fix?