[3.0] Implement name-related improvements identified in previous Vulkan PR
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:
- Handle types are now named as
-HandleEXTinstead of-EXTHandle.- Eg:
AccelerationStructureHandleKHR
- Eg:
- Function pointer delegate types are now named as
-DelegateEXTinstead ofEXTDelegate.- Eg:
pfn struct BufferCallbackSOFTandpfn delegate BufferCallbackDelegateSOFT
- Eg:
- Functions with data type suffixes following a number such as
alGetSourcei64vDirectSOFTare now trimmed and prettified asGetSourcei64vDirectSOFTinstead ofGetSourcei64VDirectSOFT. Note the capitalization ofv. This is because affixes are added after prettification. - Vendor suffixes are now kept strictly uppercased.
- OpenGL vendor suffix trimming is now generalized and can be configured and used for any Khronos API.
- Notably,
TrimEnumMemberImpliedVendorstrims enum member vendor suffixes if it matches the enum type vendor suffix.
- Notably,
- Struct properties are now prettified. They were previously ignored.
- NativeName is a new attribute that saves the name output by ClangSharp. This for the most part isn't used by the generator, but can be useful for the end user.
- NameAffix is a new attribute that allows prefixes/suffixes to be declared upfront and reordered. The idea is that mods add the attribute to identifiers and PrettifyNames applies the affixes during trimming and prettification.
- A large portion of the trimming/PrettifyNames code has been rewritten.
- NameTrimmer has been kept mostly the same.
-
INameTrimmer.Orderis used to order the trimmers instead of the version. - PrettifyNames.Visitor's functionality is mostly the same, but also gathers name affix data.
- MixKhronosData.Trim is almost all replaced by the name affix system. This is implemented in MixKhronosData.RewriterPhase3 (yes, there are 3 rewriters, but this keeps things simpler without adding much time cost).
- Most usages of tuples have been replaced with record structs.
- Most nullable collections (eg:
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. - Prettification (as in calling the Prettify extension method) is now implemented as another trimmer. Prettification now happens before name conflict resolution and before name affixes are added.
- Handle structs now have constructors since they were previously not possible to construct (field is readonly).
- The StripAttributes mod can be used to remove attributes from the final bindings.
- Currently the following are removed:
NativeTypeName,NameAffix,Transformed
- Currently the following are removed:
Related 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.
- [ ] Apply BoolType transformation to VkBool32 in structs. They currently are only handled for functions.
- [ ] Add default field values to structs where it makes sense (eg:
SType) - [ ] Ensure SupportedApiProfiles attributes are correct
- [ ] Properly resolve API profiles for Flags/FlagBits types
- [ ] Prefer name from
NativeNamefor resolving API profiles
Future PRs
- [ ] Update Curin's branch to use my new LocationTransformation code. The new renamer should be replaced with this. See the NameUtils.cs and Renamer.cs files on Curin's branch. Renamer.cs should be removed. NameUtils.cs should have the methods updated to use my LocationTransformation code instead (this should show up as a merge conflict).
- [x] PR opened and ready for review/merge
- [x] PR merged
- [ ] Old renamer code removed
- [ ] Reimplement the struct chaining API
- [ ] Figure out why the headers are getting mixed up when running multiple jobs at the same time.
- Seems to be related to
TransformFunctions.
- Seems to be related to
- [ ] Figure out why the SDL handle structs are getting named as
Silk.NET.SDL.ConditionHandle.gen.csinstead ofConditionHandle.gen.cswhen running multiple jobs at the same time. - [ ] Work on making the generator/mod configs more consistent and user friendly. This should be done before the generator is considered a public API.
- [ ] Consider opening a ClangSharp PR to fix issue with
SDL_MAX_SINT64generation on Linux- More context: https://discord.com/channels/521092042781229087/1376331581198827520/1443401810822828113
-
public const nint SDL_MAX_SINT64 = unchecked(0x7FFFFFFFFFFFFFFF);does not compile
- [ ] Consider splitting
ExtractNestedTypingandTransformHandlesinto a new set ofExtract-mods.- This this mainly for maintainability for
ExtractNestedTyping, but forTransformHandlesit is useful forAddApiProfilesto be executed after all types are extracted, but not yet transformed. - Alternatively, make
AddApiProfilesstrictly work off ofNativeNameattributes.
- This this mainly for maintainability for
- [ ] Acronym related name improvements discussed here: https://discord.com/channels/521092042781229087/587346162802229298/1446979314330501172
- [ ] Re-read discussion and organize into tasks.
- [ ] Change default acronym threshold to 2 for all bindings
- [ ] Change default acronym threshold to 2 to be the default if not specified in the PrettifyNames config.
- [ ] Handle
Mat4X4case. The "X" should be lowercased. - [ ] Update existing names to match framework design guidelines
- [ ]
EGLasEgl - [ ] Name overrides in generator config
- [ ]
- [ ] Optimize/refactor overrides in PrettifyNames
- [ ] Consider reworking data type trimming regexes.
- This was discussed more here: https://github.com/dotnet/Silk.NET/pull/2503#issuecomment-3622907776
- The summary is that the current set of 2 regexes doesn't elegantly handle the case where the ending of a word should not be trimmed, but the word itself can have data type suffixes.
- For example:
alSourceRewindvandalSourceRewind. ThedinRewindcan be mistaken as a data type suffix, so we add it to theEndingsNotToTrimregex. However, thedvinRewindvalso has the same problem. - We likely need something more akin to
WordsNotToTrimIntothanEndingsNotToTrim.
- [ ] Optimize
NativeNameattributes.- This should involve renaming
StripAttributestoCleanupAttributesand adding an option in its config to optimizeNativeNameattributes.
- This should involve renaming
- [ ] Maybe rework how using statements are added. Not sure why, but using statements are randomly added/removed between Windows/Linux. This causes noisy diffs.
Current Todos
Prepare to merge.
- [x] Finish self code review
- [x] Generate bindings on Windows for consistency
- [x] Temporarily remove generated files from PR during code review
- This needs to be done since there are too many generated files and they hide some of the manually changed files.
- [ ] Revert the commit after code review: https://github.com/Exanite/Silk.NET/pull/19
- [ ] Review the generated bindings as a second code review
- [x] Consider updating OpenAL submodule and regenerating
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
- [x] Add priority system to
INameTrimmer.- I implemented the new name affixer system using trimmers, so this introduced 3 new trimmers in order to separate each stage properly. There is now a new
INameTrimmer.Orderproperty and internalTrimmerOrderenum.
- I implemented the new name affixer system using trimmers, so this introduced 3 new trimmers in order to separate each stage properly. There is now a new
Fix misc issues I noticed
- [x]
VkVendorIdmembers are not trimmed properly - [x] Figure out why the members of the enum
AttribMaskin OpenGL are not getting rewritten.- Initial investigation shows that the
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. - Wait, are these the enums that we generate?
- Yup, they are. MixKhronosData was outputting subtly invalid type names. I changed it to use
ParseTypeName(baseType)instead ofIdentifierName(baseType).
- Initial investigation shows that the
Fix issues introduced by name affix system
- [x] Identify and fix issues introduced by name affixation. Also just identify and fix naming issues in general.
- [x] Data type trimming in OpenAL seems to be broken. Probably OpenGL as well.
- [x] Check if I need to reimplement vendor suffix removal for OpenGL. I probably do.
- [x] Discuss: I decided to change the vendor suffix removal for enum members so that it removes the member suffix if it matches the type suffix. This prevents cases where a member of a different suffix is added, causing all members to have their suffixes restored.
- [x] Discuss: The above change made it so that the member suffix removal code is usable for Vulkan so I enabled it for Vulkan as well. See PresentModeKHR to see how it looks (the KHR suffixed members have their suffixes removed, but EXT remains the same).
- [x] Reapply some of my
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.
- [x] Ensure other APIs are configured consistently with Vulkan. Eg: Ensure affix priorities are set, etc.
Name affix system
- [x] Properly trim prefixed/suffixed names, such as
PFNVkDebugUtilsMessengerCallbackEXTandBufferTHandle- [x]
PFNVkDebugUtilsMessengerCallbackEXTprefix - [x]
PFNVkDebugUtilsMessengerCallbackDelegateEXTsuffix - [x]
AccelerationStructureHandleKHRsuffix - [x] Extract vendor extensions from names and store into
NameSuffixattributes. - [x] Revert "hacky" THandle fix.
- [x]
- [x] Consider renaming handle types as
HandleEXTinstead ofEXTHandlefor readability.- This is fairly doable. The above todos track this.
Name metadata
- [x] Add NativeName attribute
- [x] Ensure NativeName attribute is added by all mods that generate new types/members
- Technically not strictly required, but nice from a API consistency standpoint.
- [x] MixKhronosData
- [x] Enum members
- [x] Enum declarations
- [x] Vulkan (uses FlagBits version of enum names since it is more helpful when googling)
- [ ] Discuss validity in other Khronos APIs (some APIs don't have actual enums)
- Eg:
GLEnumand the other OpenGL enums
- Eg:
- [x] ExtractNestedTyping
- [x] Function pointers
- [x] Others?
- [x] TransformHandles
Name prettification
- [x] ~~Change naming convention to not capitalize acronyms (i.e., strictly pascal cased)~~
- ~~See: https://discord.com/channels/521092042781229087/1376331581198827520/1395097379329282089~~
- [x] ~~Update generated method class names in the rsp files to be strictly pascal case~~
- [x] Fix
StdVideoEncodeH264SliceHeaderFlagsproperty names. They currently aren't getting prettified.
Fix bugs from original PR
- [x] Enums containing negative values should not have an unsigned backing type.
Handle struct improvements
- [x] Handle structs don't have an easy way of being constructed (field is readonly and no constructor).
Ready for review!
If you want to see the generated code, see: ~~https://github.com/dotnet/Silk.NET/pull/2503/commits/9c9fbfc037b662a260ff5c8eccbdeda4c064cc78.~~ Edit: This is probably easier to read: https://github.com/Exanite/Silk.NET/pull/18
Note that the red/removed code is the new code since this code is meant to temporarily revert the generated changes.
Going to revert acronym casing changes as discussed in the Discord before reopening PR for review.
Not sure why I can't reply to the following comment:
Why didn't the EndingsNotToTrim regex immediately below this not pick it up?
But the reason for this is because ...Radix|Sound|Rewind|Supported)$ matches only if Rewind is at the very end of the name.
Rewind is also a function name that has data type suffixes.
That being said, the only suffix it does have is -v and it never truly gets trimmed due to name conflict so perhaps we can just add Rewindv as part of EndingsNotToTrim. This is notably dangerous if OpenAL decides to add a different, actually trimmable suffix (I'm not familiar with OpenAL so I'm not sure that they will though).
Two other things we probably should consider:
- Changing it so that the regex is configurable in the generator config.
- Adding a comment explaining why
(?<!Rewin)[dhf]v?is part ofEndingsToTrim. - (Same as above, but for completeness), consider adding
Rewindvas part ofEndingsNotToTrim.
This is mostly my concern, EndingsNotToTrim was intended to provide the "do the data type trimming, but prevent these special cases from being trimmed". If the new code is no longer using EndingsNotToTrim in this way, I'd rather fix that than add special cases to EndingsToTrim.
With regards to Rewindv, I would expect it to behave as it does on develop as well i.e. Rewindv is added as a secondary. I don't expect the fix to be adding Rewindv to EndingsNotToTrim.
I probably should have mentioned that it's also wrong on develop/3.0:
This was a case that I noticed and fixed; not a case that I introduced and fixed.
Oh... lol. Ok yeah maybe this is the appropriate fix.
On second thought though, I think my statement here is wrong:
This is notably dangerous if OpenAL decides to add a different, actually trimmable suffix (I'm not familiar with OpenAL so I'm not sure that they will though).
I think it's safe to add Rewindv to EndingsNotToTrim since it only prevents the -v suffix from being trimmed.
Yeah I agree. Maybe we should integrate the words in EndingsNotToTrim into EndingsToTrim to make sure we don't run into another spot of bother in the future.
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 Rewind before matching for dv or v.
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 Rewindv to EndingsNotToTrim for now.
For the record, the PR is ready to review again, but I'm currently doing some additional reviewing of the generated bindings to ensure that I haven't changed anything that I didn't mean to change.