-
-
Notifications
You must be signed in to change notification settings - Fork 439
[3.0] Implement name-related improvements identified in previous Vulkan PR #2503
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop/3.0
Are you sure you want to change the base?
[3.0] Implement name-related improvements identified in previous Vulkan PR #2503
Conversation
This looks like it's from some sort of built-in renamer. Don't think we'll ever use this since we have a custom renamer.
… work with native name attributes
Not sure why these were not being coerced when the Vulkan PR was merged, but they started getting coerced in this PR. This technically is a good thing since it means the coerce backing types code is now working as previously intended. However, the previously intended implementation missed an edge case, which this commit fixes.
This is because the structs were impossible to reasonably construct before.
VaListTagHandle is an extra handle struct that seems to generate on Linux (and not on Windows) and breaks global prefix determination.
OpenAL: ContextErrorCode seems to always be replaced with ErrorCode, even on Windows. Not sure why. This also happened during the last PR, but I didn't commit it.
…name trimmer code
Honestly, what I wrote is a complete guess, I'll likely revisit these once I understand them more.
b06e648 to
53fbdd3
Compare
c6a07d7 to
9bbf8be
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self code review complete. Going to mark as ready for review.
Note that I only have the non-generated changes committed.
The generated changes are here: Exanite#19
This is because the generated changes hide the non-generated changes (it goes over the 1k file limit).
Feel free to comment on the linked PR as well.
It will be merged into this PR after the non-generated changes are reviewed.
DO NOT merge this PR until the generated changes are merged, and preferably, reviewed.
| return attributeLists; | ||
| } | ||
|
|
||
| return attributeLists.WithNativeName(identifier.Text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should change this to not apply NativeName if:
- the native name is identical to the current name, and
- it is obvious that the symbol is representing a native symbol (i.e. one of the parent syntax nodes has a
NativeName)
This way NativeName is still used to signify that this a representation of a native symbol, but where that's already known, we're not adding attributes that aren't conveying any extra information (such as when the names are identical)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be something we do as a post-process step and not at a pre-process step.
MarkNativeNames is designed as a pre-process step. It goes early in the mod order.
MarkNativeNames goes right after ClangScraper because, at that point, we know that all names match the native names (outside some exceptions like in bitfield structs where ClangSharp introduces an extra field).
Attribute removal is currently handled by StripAttributes, which is a post-process step. It goes late (last currently) in the mod order.
This mod can be generalized as an CleanupAttributes mod where optimizing attributes is part of the configurable steps that the mod applies.
The reason why optimizing attributes should be done as a post-process is because optimization requires knowledge of the output. If we introduce the "don't add if redundant" logic during pre-processing, then no NativeName attributes would ever get added.
An alternative is for mods to only add the NativeName attribute if they change the name, but that also requires all mods to understand the rules for adding/removing the NativeName attribute, which greatly increases complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmmm ok, I agree - I must admit I haven't checked the bindings yet so I don't know whether this is being done already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The optimization of NativeName attributes isn't, but it's not too hard to implement.
To me, this is a lower priority task since it's not critical to the bindings working.
| @@ -1800,7 +1505,7 @@ public bool TryGetSymbolMetadata( | |||
| /// while the lookbehind asserts that the ending match will not overreach into the end of a word. | |||
| /// </summary> | |||
| // NOTE: LET THIS BE A LESSON! Do NOT add x (fixed) here, these will frequently conflict with integer overloads. | |||
| [GeneratedRegex("(?<!xe)([fdh]v?|u?[isb](64)?v?|v|i_v|fi|hi)$")] | |||
| [GeneratedRegex("(?<!xe)((?<!Rewin)[dhf]v?|u?[isb](64)?v?|v|i_v|fi|hi)$")] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why didn't the EndingsNotToTrim regex immediately below this not pick it up?
|
|
||
| public override SyntaxNode VisitEnumDeclaration(EnumDeclarationSyntax node) | ||
| { | ||
| // Special case for enums since this code needs information about the enum type and its members at the same time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it need to know its type?
| exclusiveVendor = null; | ||
| } | ||
|
|
||
| // Check if the enum contains unsuffixed members |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
| return false; | ||
| }); | ||
|
|
||
| var isSafeToTrimMembers = !containsUnsuffixed; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does it not containing any unsuffixed vendors make it safe? How is this different to exclusiveVendor?
|
|
||
| if (config.TrimEnumTypeNonExclusiveVendors && typeVendor != null) | ||
| { | ||
| var shouldTrimType = typeVendor != exclusiveVendor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't keep asking questions out loud, but generally I think this code could do with some more comments with some examples of what it's actually doing as it's quite hard to visualise.
I think what it's doing makes sense but it requires a lot of thought and careful reading to figure out what it's doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code mostly matches the functionality of the code that was originally in MixKhronosData.Trim.
Well... to the best of my ability since that code wasn't really commented either.
The exceptions are that I removed/prevented some edge cases and restructured the code.
Edit: Another exception is that I changed the enum member name trimming condition to be if the member matches the enum type name vendor suffix. Previously it was if all member names match the type vendor suffix (hence the config option being called TrimEnumMemberImpliedVendors instead of TrimEnumMemberSharedVendors). This was the part I discussed with you in the discord.
I provide a better explanation of the functionality of the code in the Configuration record struct at the top of the class, but I agree that that this code is likely hard to understand without context and local examples. I'll go through the code and add these.
| { | ||
| if (isSafeToTrimType) | ||
| { | ||
| // Remove the exclusive vendor from the enum name since it is wrong and it is safe to do so |
There was a problem hiding this comment.
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 do so? Why is it wrong? Can you give an example of where this code is used? This could result in breaking changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the code to answer these questions. Same for the similar review comments in this area of the code.
This is how this section now looks:

I'm not exactly sure what you mean by breaking changes since this behavior exactly matches the original behavior of the code as it as in MixKhronosData.Trim. Keep in mind that this isn't new functionality. This is old functionality ported over to the new name affix system.
That being said, both the old and new implementations do have the possibility of introducing breaking changes if upstream introduces another variant of the enum that prevents the vendor suffix from getting trimmed.
For example, CombinerRegisterNV currently exists and has the NV vendor suffix trimmed due to it containing GL_TEXTURE0_ARB, similar to this example:

If Khronos was to add CombinerRegisterAMD, then CombinerRegisterNV's suffix would no longer be trimmed and CombinerRegister would no longer exist as an enum in the generated bindings.
That being said, this is probably a rare case.
Note that if Khronos added CombinerRegister, there likely would be no breaking change.
| /// Used to define the order that the trimmers will run in. | ||
| /// Higher values indicate that the trimmer should run later. | ||
| /// </summary> | ||
| int Order { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be the way we determine the order of execution - this is not something individual augments should be able to influence without careful consideration of what it's trying to achieve. If the order of execution matters and has different effects, this should be user configurable. As I've said before, I want to get rid of things like Version where you arbitrarily change the number to achieve the result you want - if there is a need to control the order of execution to achieve the results the user wants, then that's what the user-facing design should reflect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fully agree. In this case, I only documented the functionality of the property, but I probably should have also documented the intended usage.
This is the intended usage:
Mods should only use the values -1 or 0 for Order and DiscriminatorPriority.
-1 corresponds to "disabling" the property and 0 corresponds to leaving it as the default value.
All other values should come from the generator config.
Mods also should not attempt to compute what values to use for Order and DiscriminatorPriority.
The above usage matches how the properties are currently used by the mods in my PR.
Arguably, even 0 and -1 should not be hardcoded, but I opted not to do that since it would expose a lot of configuration properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, I completely misunderstood what code this comment was addressing.
I thought it was referring to the NameAffix attribute. For that case, my comment still holds and I'll still update the documentation to include the intended usage of the attribute.
As for the trimmer case though, I'm not sure.
The order property here, as it currently stands, is for ensuring correctness rather than for allowing for customizability. The affixes have to trimmed in a certain order for my NameAffix system to work, as seen here.

However, for trimmers not part of the required set (eg: the trimmers defined by mods), those probably should have their own configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only external trimmer (external as in not part of PrettifyNames) here is MixKhronosData.
This means we currently don't really have a usage pattern to analyze.
What other trimmers will there be that isn't covered by the existing functionality?
Even NameTrimmer can probably use the name affix system instead, which has a well defined and configurable execution order. From what I understand, it mainly identifies and trims global prefixes, so it can be implemented as a IdentifyGlobalPrefixes mod that has a syntax rewriter. This also lets configure whether global prefixes should be identified; if they should be identified, but kept; and if they should be kept, but reordered.
MixKhronosData does provide one case, which is to prevent/rewind trimming in the case of GLEnum.
Generally speaking, this would be more of a name replacement/transformation. This is not covered by the affix system, but is partially covered by the name override system.
|
Not sure why I can't reply to the following comment:
But the reason for this is because
Two other things we probably should consider:
|
|
This is mostly my concern, With regards to |
|
Oh... lol. Ok yeah maybe this is the appropriate fix. |
|
On second thought though, I think my statement here is wrong:
I think it's safe to add |
|
Yeah I agree. Maybe we should integrate the words in |
|
As in, combine the two regexes? I think that would make it confusing to edit. Edit: I think I see what you mean. You mean to make it so that the regex considers if trimming the suffix would trim into part of another word. Eg: Match for |
|
I'm concerned about only having Rewind in EndingsToTrim if that is indeed the valid fix. I still feel like there should be a way to use EndingsNotToTrim to get what we want but I'm not sure how. But I don't want the two regexes to not enact the correct behaviour because they have different sets of applicable words. |
|
I added this to my task tracker above to be done in a separate PR. Don't resolve the PR comment just yet though since I think it is better to add |

Summary of the PR
This PR focuses on implementing the naming-related improvements identified in #2457.
Depending on the scope of the tasks, some of these tasks may be pushed to another PR.
Important changes:
-HandleEXTinstead of-EXTHandle.AccelerationStructureHandleKHR-DelegateEXTinstead ofEXTDelegate.pfn struct BufferCallbackSOFTandpfn delegate BufferCallbackDelegateSOFTalGetSourcei64vDirectSOFTare now trimmed and prettified asGetSourcei64vDirectSOFTinstead ofGetSourcei64VDirectSOFT. Note the capitalization ofv. This is because affixes are added after prettification.TrimEnumMemberImpliedVendorstrims enum member vendor suffixes if it matches the enum type vendor suffix.INameTrimmer.Orderis used to order the trimmers instead of the version.List<string>?) have been made non-nullable. Most of these get allocated anyway and it makes the code more maintainable. If there is any performance issue (mainly concerning the Microsoft job), I will take the time to optimize it.NativeTypeName,NameAffix,TransformedRelated issues, Discord discussions, or proposals
Previous Vulkan PR (initial bindings generation): #2457
Discord thread: https://discord.com/channels/521092042781229087/1376331581198827520/1442651368207941643
Further Comments
Tasks not part of this PR
These are the tasks from my previous PR. These have been sorted and trimmed down to ensure that this PR remains focused. These may be added to this PR.
If you want to see the original, unmodified set of tasks, please see #2457.
These will also be part of a future PR.
SType)NativeNamefor resolving API profilesFuture PRs
TransformFunctions.Silk.NET.SDL.ConditionHandle.gen.csinstead ofConditionHandle.gen.cswhen running multiple jobs at the same time.SDL_MAX_SINT64generation on Linuxpublic const nint SDL_MAX_SINT64 = unchecked(0x7FFFFFFFFFFFFFFF);does not compileExtractNestedTypingandTransformHandlesinto a new set ofExtract-mods.ExtractNestedTyping, but forTransformHandlesit is useful forAddApiProfilesto be executed after all types are extracted, but not yet transformed.AddApiProfilesstrictly work off ofNativeNameattributes.Mat4X4case. The "X" should be lowercased.EGLasEglalSourceRewindvandalSourceRewind. ThedinRewindcan be mistaken as a data type suffix, so we add it to theEndingsNotToTrimregex. However, thedvinRewindvalso has the same problem.WordsNotToTrimIntothanEndingsNotToTrim.NativeNameattributes.StripAttributestoCleanupAttributesand adding an option in its config to optimizeNativeNameattributes.Current Todos
Prepare to merge.
Completed Todos
Newest groups at the top. Order within a group is chronological.
Note that I also wrote a summary at the top. The summary is more comprehensive, but this section can be useful getting additional context. Also see my commit descriptions if more context is needed.
INameTrimmer.Order
INameTrimmer.INameTrimmer.Orderproperty and internalTrimmerOrderenum.Fix misc issues I noticed
VkVendorIdmembers are not trimmed properlyAttribMaskin OpenGL are not getting rewritten.fieldSymbol.ConstantValueis null, but why it is null is unclear. Maybe a Roslyn bug? Example member:DepthBufferBit = unchecked((uint)0x00000100). Note that similar expressions in Vulkan are fine, eg:unchecked((ulong)0x00000002UL). Maybe it is because the literal type and cast type are different.ParseTypeName(baseType)instead ofIdentifierName(baseType).Fix issues introduced by name affix system
MixKhronosDatacode removals to the currentdevelop/3.0branch and see what changes would be made. This is to ensure I don't accidentally remove functionality when deleting code.Name affix system
PFNVkDebugUtilsMessengerCallbackEXTandBufferTHandlePFNVkDebugUtilsMessengerCallbackEXTprefixPFNVkDebugUtilsMessengerCallbackDelegateEXTsuffixAccelerationStructureHandleKHRsuffixNameSuffixattributes.HandleEXTinstead ofEXTHandlefor readability.Name metadata
GLEnumand the other OpenGL enumsName prettification
Change naming convention to not capitalize acronyms (i.e., strictly pascal cased)See: https://discord.com/channels/521092042781229087/1376331581198827520/1395097379329282089Update generated method class names in the rsp files to be strictly pascal caseStdVideoEncodeH264SliceHeaderFlagsproperty names. They currently aren't getting prettified.Fix bugs from original PR
Handle struct improvements