-
-
Notifications
You must be signed in to change notification settings - Fork 438
[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.
…vulkan-bindings-3.0-data-type-trimming
…yway. Will profile and optimize later.
b1c03ee to
f75dd7f
Compare
Exanite
left a comment
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.
Initial self code review. PR is not yet ready for review or merge.
| openal-soft-al.h | ||
| --methodClassName | ||
| AL | ||
| Al |
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 replaced most usages of AL and GL with Al and Gl (I still need to double check comments since my renames don't catch that).
However, I did not change namespaces, so they still use OpenAL/GL instead of OpenAl/Gl.
This is more consistent with the rest of the bindings, but I'm not sure if we want to make an exception here and use GL instead. Notably SDL is already lowercased as Sdl so I didn't change that.
|
|
||
| namespace Silk.NET.OpenAL; | ||
|
|
||
| [NativeName("_flLateReverbPan_e__FixedBuffer")] |
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.
Not sure how I feel about this native name. Probably fine?
| [InlineArray(3)] | ||
| public partial struct EfxEaxReverbPropertiesFlLateReverbPan | ||
| { | ||
| [NativeName("e0")] |
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.
Also not sure how I feel about this native name. Probably fine?
| @@ -37,42 +37,43 @@ public static partial class NameUtils | |||
| "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_" | |||
| ); | |||
|
|
|||
| private static readonly IStringTransformer[] _prettifyTransformers = [new NameTransformer()]; | |||
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 avoids an allocation due to the params keyword every time Prettify is called.
| /// </summary> | ||
| /// <param name="node">The original method.</param> | ||
| /// <returns>The modified method.</returns> | ||
| public static MethodDeclarationSyntax WithRenameSafeAttributeLists( |
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.
All Attribute related code in ModUtils has been moved to AttributeUtils
| /// <inheritdoc /> | ||
| public void Trim(NameTrimmerContext context) | ||
| { | ||
| if (context.Names is null || context.JobKey is null) |
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.
Most of Trim was replaced by RewriterPhase3 below
| @@ -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.
Rewindv in OpenAL was getting trimmed as Rewin
| private static readonly char[] _listSeparators = { ',', '|', '+' }; | ||
|
|
||
| private static readonly Dictionary<string, string> _defaultEnumNativeTypeNameMaps = |
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 dictionary was effectively unused.
Also, apparently I didn't regenerate OpenGL following some of my previous changes. The data type suffixes in particular have changed.
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:
GL, exists outside of generated code.GLbecomesGl.RGBbecomesRgb-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.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
StdVideoEncodeH264SliceHeaderFlagsproperty names. They currently aren't getting prettified.Fix bugs from original PR
Handle struct improvements
Things to watch for during review
I regenerated all of the bindings, but on Linux. I'll probably commit the Windows version before undrafting for consistency.