[3.0] Improve handling of affixed names when singularizing (OES/SGIS issue) and expose NameAffix API #2535
Conversation
Exanite
changed the title
… prefix; Rewrite StripAffixes to not allocate; Update PrettifyNames to use new GetAffixes method
Exanite
force-pushed
the
fix/vendor-suffix-singularize-interaction
branch
from
6b7c293 to
589792e
Compare
… the AddNameAffix method instead of AddNameSuffix/Prefix separately
Exanite
left a comment
Exanite
left a comment
There was a problem hiding this comment.
Self review completed
| /// Use false if not (outside or appended to end). | ||
| /// True means that the attribute is added to the start of the attribute list, meaning that the affix is re-appended earlier. | ||
| /// </param> | ||
| public static SyntaxList<AttributeListSyntax> AddNamePrefix( |
There was a problem hiding this comment.
I moved these to the new NameAffixer static class to keep everything related to name affixes together.
I also changed the API to use the new NameAffixType enum and changed it so that we only have the AddNameAffix method instead of AddNamePrefix/Suffix separately.
| affixes | ||
| ); | ||
|
|
||
| return node.WithIdentifier(Identifier(singularizedIdentifier)) |
There was a problem hiding this comment.
This is the core fix for the OES/SGIS singularize issue.
| private record struct NameAffix( | ||
| bool IsPrefix, | ||
| string Category, | ||
| string Affix, | ||
| int DeclarationOrder |
There was a problem hiding this comment.
This been moved to a separate file as a public type.
| || _enumInProgress is not null | ||
| || node.Ancestors().OfType<BaseTypeDeclarationSyntax>().Any(); | ||
|
|
||
| private bool TryGetAffixData( |
There was a problem hiding this comment.
Replaced by NameAffixer.GetNameAffixes(), which returns an array directly instead of the TryGet pattern. The array is empty in the "false" case and does not allocate.
| /// <param name="affixTypes">The affix data retrieved by the <see cref="Visitor"/>.</param> | ||
| /// <param name="config">The configuration from <see cref="Configuration.Affixes"/>.</param> | ||
| private class NameAffixer( | ||
| private class PrettifyNamesAffixer( |
There was a problem hiding this comment.
This class is the NameAffixer, but specialized for the needs of PrettifyNames.
| [ | ||
| .. affixes, | ||
| new NameAffix( | ||
| type == "Prefix" ? NameAffixType.Prefix : NameAffixType.Suffix, | ||
| category, | ||
| affix, | ||
| declarationOrder | ||
| ), | ||
| ]; |
There was a problem hiding this comment.
Not sure what is more performant here. Most names right now have 0 affixes so this method won't allocate at all.
Later, if we change NameTrimmer to be a mod that adds name affixes, most names will have 1 affix (the shared prefix), which would allocate, but would be the minimal amount.
Also not sure if this gets stack allocated or not by .NET 10. I need to look up the escape analysis rules.
| /// <summary> | ||
| /// The type of name affix. | ||
| /// </summary> | ||
| public enum NameAffixType |
There was a problem hiding this comment.
I decided to add this enum since the name affix API is now fully exposed.
1 participant
Summary of the PR
This fixes the interaction between Khronos vendor suffixes and the Singularize method, which led to cases like the following:
In order to fix this, this also exposes the NameAffix API, allowing mods to read NameAffix attributes and process the name without the affixes attached.
Related issues, Discord discussions, or proposals
Original Discord discussion: https://discord.com/channels/521092042781229087/1376331581198827520/1456785841753035005
Further Comments
Todo list:
OESand similar from being singularizing intoOinArrayParameterTransformer.TransformArrayParameterRewriter.glFeedbackBufferxOESTransformArrayParameterRewritercan singularize the base name instead of the full name.OES(incorrect version isOOES)SGIS(incorrect version isSgiSGIS)NameAffixdata.AddNameAffixAPI to use the newNameAffixTypeenum for consistency and simplicity