Cleanup handling of flags in internal syntax nodes. #72130
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
| @@ -24100,13 +24100,13 @@ internal abstract partial class DirectiveTriviaSyntax : StructuredTriviaSyntax | |||
| internal DirectiveTriviaSyntax(SyntaxKind kind, DiagnosticInfo[]? diagnostics, SyntaxAnnotation[]? annotations) | |||
| : base(kind, diagnostics, annotations) | |||
| { | |||
| this.flags |= NodeFlags.ContainsDirectives; | |||
| SetFlags(NodeFlags.ContainsDirectives); | |||
There was a problem hiding this comment.
We had a mismash of callers either directly manipulating the flags, or calling the helpers. Now, everyone uses the helpers (teh core flags are not exposed now either).
| @@ -175,7 +175,7 @@ internal static DirectiveStack ApplyDirectivesToListOrNode(GreenNode listOrNode, | |||
|
|
|||
| internal virtual IList<DirectiveTriviaSyntax> GetDirectives() | |||
| { | |||
| if ((this.flags & NodeFlags.ContainsDirectives) != 0) | |||
| if (this.ContainsDirectives) | |||
There was a problem hiding this comment.
a tiny handful of cases were examining the flags, instead of the existing api that more easily and correctly checks these cases.
| get => _nodeFlagsAndSlotCount.SmallSlotCount; | ||
| set => _nodeFlagsAndSlotCount.SmallSlotCount = value; | ||
| } | ||
| private NodeFlags Flags => _nodeFlagsAndSlotCount.NodeFlags; |
There was a problem hiding this comment.
this is now private. No need for anything outside of this type to access this value. All functionality is already exposed through clearer properties, or helper methods.
| ContainsSkippedText = 1 << 8, | ||
| ContainsStructuredTrivia = 1 << 9, | ||
|
|
||
| InheritMask = IsNotMissing | ContainsAnnotations | ContainsAttributes | ContainsDiagnostics | ContainsDirectives | ContainsSkippedText | ContainsStructuredTrivia, | ||
| } | ||
|
|
||
| internal void SetFlags(NodeFlags flags) |
There was a problem hiding this comment.
note: i wanted to make these protected. That works in C#, but vb has a few places taht call these directly on a node (odd).
this is also nice as we now only expose the ability to set/clear bits. not the ability to accidentally assign over the node-flags entirely.
| } | ||
|
|
||
| internal bool IsMissing | ||
| { | ||
| get | ||
| { | ||
| // flag has reversed meaning hence "==" | ||
| return (this.flags & NodeFlags.IsNotMissing) == 0; | ||
| return (this.Flags & NodeFlags.IsNotMissing) == 0; |
There was a problem hiding this comment.
properly renamed to represent that this is a property.
| // Note: it's ok to examine SmallSlotCount here. All we're trying to do is make sure we have at least one | ||
| // child. And SmallSlotCount works both for small counts and large counts. This avoids an unnecessary | ||
| // virtual call for large list nodes. | ||
| while (node?._nodeFlagsAndSlotCount.SmallSlotCount > 0); |
There was a problem hiding this comment.
this code was def odd (the direct use of _slotCount). but it's technically totally ok to do that. i've added docs to say why.
| @@ -103,7 +103,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax | |||
|
|
|||
| Friend Sub New(ByVal kind As SyntaxKind, empty As InternalSyntax.PunctuationSyntax) | |||
| MyBase.New(kind) | |||
| MyBase._slotCount = 1 | |||
There was a problem hiding this comment.
vb overrode this, instead of using the existing SlotCount property. C# uses SlotCount. This brings the generated code in sync.
| @@ -83,20 +83,6 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.Syntax.InternalSyntax | |||
| End Get | |||
| End Property | |||
|
|
|||
| Protected Overrides Function GetSlotCount() As Integer | |||
| Throw ExceptionUtilities.Unreachable | |||
| End Function | |||
There was a problem hiding this comment.
this logic moved up. both vb and c# should share it.
| Set(value As Integer) | ||
| Me.SlotCount = value | ||
| End Set | ||
| End Property |
There was a problem hiding this comment.
such a bizarre property. it existed just to forward to SlotCount... so i've removed it entirely.
| } | ||
|
|
||
| /// <inheritdoc cref="NodeFlagsAndSlotCount.SmallSlotCount"/>> | ||
| private byte _slotCount |
There was a problem hiding this comment.
since this was private, i just inlined it into the tiny handful of cases in this code that referenced it.
| get => _nodeFlagsAndSlotCount.SmallSlotCount; | ||
| set => _nodeFlagsAndSlotCount.SmallSlotCount = value; | ||
| } | ||
| internal NodeFlags Flags => _nodeFlagsAndSlotCount.NodeFlags; |
There was a problem hiding this comment.
I'd like to make this private. But VB has a couple of usages realted to caching tokens where it directly introspects this flag.
|
@RikkiGibson @333fred ptal. |
|
This it eh last flag-related pr i expect ot make for a while. |
|
Done with review pass (commit 9) |
|
I believe feedback is resolved. |
ghost
added this to the 
Followup to #72104
Things i noticed and wanted to clean up, but weren't strictly necessary for the core changes in those other prs.