Move SourceOrdinaryMethodSymbol functionality that is not syntax specific into a new base class.#45424
Conversation
…ific into a new bas class. This work is related to dotnet#45296.
|
@jcouv, @agocke, @cston , @RikkiGibson, @dotnet/roslyn-compiler Please review. #Closed |
|
@jcouv, @agocke, @cston , @RikkiGibson, @dotnet/roslyn-compiler Please review. #Closed |
|
Taking a look. #Resolved |
|
Looking. Any tips on how to review this change? #Resolved |
The code was basically split between two files with some minor adjustments. I review changes like that by looking at two diffs in parallel. For this PR, I would look at the diffs:
I usually use WinDiff for that, but any diff tool would do. Stat with the first commit for these files. Second is just minor changes that, I think,, easier to review separately. #Resolved |
|
|
||
| protected override void ExtensionMethodChecks(DiagnosticBag diagnostics) | ||
| { | ||
| var syntax = GetSyntax(); |
There was a problem hiding this comment.
Should these locals be declared inside the if (IsExtensionMethod) { block instead? #Pending
This can also be done with GitLens in VS Code by:
|
| } | ||
| } | ||
|
|
||
| protected virtual void CompleteAsyncMethodChecksBetweenStartAndFinish() |
There was a problem hiding this comment.
nit: Consider abstract and when the new derived type comes, it can have an empty body (won't need to raise event for declared symbol). #Pending
| } | ||
| return mods; | ||
| var syntax = GetSyntax(); | ||
| return ModifierUtils.MakeAndCheckNontypeMemberModifiers(syntax.Modifiers, defaultAccess: DeclarationModifiers.None, allowedModifiers, Locations[0], diagnostics, out _); |
There was a problem hiding this comment.
nit: consider naming discarded argument #Resolved
There was a problem hiding this comment.
nit: consider naming discarded argument
I think I intentionally made it discard.
In reply to: 445753835 [](ancestors = 445753835)
|
|
||
| namespace Microsoft.CodeAnalysis.CSharp.Symbols | ||
| { | ||
| internal abstract class SourceOrdinaryMethodSymbolBase : SourceMemberMethodSymbol |
There was a problem hiding this comment.
SourceOrdinaryMethodSymbolBase [](start = 28, length = 30)
nit: May be worth leaving a comment explaining the syntax vs. no-syntax purpose of the base and derived types. #Pending
RikkiGibson
left a comment
There was a problem hiding this comment.
LGTM besides pending comments
|
@jcouv, @RikkiGibson Thank you for your feedback. I am proceeding with merging this PR as is. I will address the "pending" suggestions in the next PR that I already started working on. |
| bool isIterator, | ||
| bool isExtensionMethod, | ||
| bool isPartial, | ||
| bool hasBody, |
There was a problem hiding this comment.
Do we want to deal with hasBody in the derived type? It would mean the derived types have to do more work, but it kind of moves the work to where it belongs -- alongside the syntax.
Not pressing, I don't think it really matters, but if we wanted a stronger separation between syntax and semantics (which I think is good and would make stuff like this easier in the future), that might be something to consider. #Resolved
There was a problem hiding this comment.
Do we want to deal with this in the derived type? It would mean the derived types have to do more work, but it kind of moves the work to where it belongs -- alongside the syntax.
As we discussed offline. At the moment, we can take advantage of code sharing for reporting diagnostics that is specific for methods that have a body.
For methods, if they have a declaration syntax, the presence of a body is a syntax check. The synthesized ones know if there is a body based on some specific situation and we want to perform the checks for them too.
In reply to: 445817294 [](ancestors = 445817294)
This work is related to #45296.