[3.0] Replace NameTrimmer with equivalent IdentifySharedPrefixes mod#2557
Closed
Exanite wants to merge 82 commits intodotnet:develop/3.0from
Closed
[3.0] Replace NameTrimmer with equivalent IdentifySharedPrefixes mod#2557Exanite wants to merge 82 commits intodotnet:develop/3.0from
Exanite wants to merge 82 commits intodotnet:develop/3.0from
Conversation
…ld used by other bindings The default acronym threshold was changed during #29, which was merged as part of dotnet#2503.
Considering we decided to follow Microsoft's Framework Design Guidelines (acronym threshold of 2) for the bindings and rest of the API, might as well be consistent here.
… separation hack (no longer necessary)
…mbers are normalized
This lets us handle prefixing and prettification separately, which notably is important if we add prefixes after prettification. We want to prefix the final name, not the intermediate name in this case.
…igured (cherry picked from commit bc94260)
…ous that it is not the last trimmer
This no longer makes sense to keep and enabling features by baseline version seems fiddly. If we need to toggle features for newer versions, we can explicitly add a boolean config option.
…essor and ReapplyAffixesNameProcessor
Kinda a cop out decision, but it keeps thing simple (and thus maintainable) and implementing it fully seems overkill for what we need.
… for function pointer types are named
This is because we no longer output the separating underscore in ExtractNestedTyping
… of INameTrimmer registrations
…ed-struct-name-affixes
…nvention Note that the goal is to eventually remove the name overrides for the `EFXEAXREVERBPROPERTIESflLateReverbPan` and `-Delegate` cases entirely. This is because these are theoretically possible to handle automatically and the reason it doesn't work is due to an edge case interaction with the name override system. See the "Tasks" section here for more info: dotnet#2555
This reverts commit 3ec90a7.
This is because 2D isn't trimmed properly by my new name splitter, but I decided that this case was not worth handling in favor of keeping the trimming code simple. See my thoughts here: https://discord.com/channels/521092042781229087/1485943448069738608/1489457083857506336
…amesTests to IdentifySharedPrefixesTests
See dotnet#2557 for full reasoning, but the short is that this edge case is purely theoretical and can be handled later on when actually necessary.
This was referenced Apr 7, 2026
Exanite
added a commit
that referenced
this pull request
Apr 14, 2026
* Update OpenAL name overrides to match the 2 character acronym threshold used by other bindings The default acronym threshold was changed during Exanite#29, which was merged as part of #2503. * Start of feature/nested-struct-name-affixes branch * Cleanup leftover return statement * Split TransformHandles into two mods * Rename UseDSL to UseDsl Considering we decided to follow Microsoft's Framework Design Guidelines (acronym threshold of 2) for the bindings and rest of the API, might as well be consistent here. * Cleanup unused property in TransformHandles * Cleanup usages of redundant collections expressions in TransformHandles * Add NameAffixer.AddResolvedNameAffix() for handling compound names * Use AddResolvedNameAffix for extracted function pointer delegate types * Use AddResolvedNameAffix for extracted nested structs and remove name separation hack (no longer necessary) * Rename resolved affix to referenced affix and add parsing code for referenced affixes * Add extra ConsecutiveNumbers test case documenting that Xs between numbers are normalized * Fix incorrect declarationOrder implementation * Change Prettify to not prefix identifiers starting with numbers This lets us handle prefixing and prettification separately, which notably is important if we add prefixes after prettification. We want to prefix the final name, not the intermediate name in this case. * Change the list and order of INameTrimmers used to be statically configured (cherry picked from commit bc94260) * Replace now outdated comment * Move PrettifyNamesTrimmer higher up in the class to make it more obvious that it is not the last trimmer * Remove INameTrimmer.Version This no longer makes sense to keep and enabling features by baseline version seems fiddly. If we need to toggle features for newer versions, we can explicitly add a boolean config option. * Add PrefixIfStartsWithNumberTrimmer * Remove TrimmerBaseline config option * Rename INameTrimmer to INameProcessor and update rest of codebase * Simplify name affixer name processor names to be StripAffixesNameProcessor and ReapplyAffixesNameProcessor * Shorten names a bit more for readability * Decide to only support references within the same scope depth Kinda a cop out decision, but it keeps thing simple (and thus maintainable) and implementing it fully seems overkill for what we need. * Restrict feature set of AddReferencedNameAffix even more for simplicity * Implement referenced affixes and change how -Delegate types generated for function pointer types are named * Update OpenAL name overrides in generator.json This is because we no longer output the separating underscore in ExtractNestedTyping * Update doc comment in ServiceCollectionExtensions to indicate removal of INameTrimmer registrations * Update name overrides for OpenAL to match new Delegate type naming convention Note that the goal is to eventually remove the name overrides for the `EFXEAXREVERBPROPERTIESflLateReverbPan` and `-Delegate` cases entirely. This is because these are theoretically possible to handle automatically and the reason it doesn't work is due to an edge case interaction with the name override system. See the "Tasks" section here for more info: #2555 * Optimize comparison by hoisting the calculation * Add topological sort to ReapplyAffixesProcessor This covers cases where there are multiple levels of nested structs, such as in SDL: GamepadBinding contains GamepadBindingInput which contains GamepadBindingInputAxis * Improve error handling by checking for case where dependencies are not fully fulfilled * Edit error message * Add test: PrettifyNamesTests.SuccessfullyUsesReferencedAffixes * Use "SDL.gen.cs" for test document name This is just to hint that these structs come from SDL * Add tests for referenced affix behavior MissingReferencedAffix_Throws currently fails, but seems to be an unrelated issue. * Alphabetically sort the mod names * Add start of IdentifySharedPrefixes mod * Work on IdentifySharedPrefixes * Remove ApplyPrettifyOnlyPipeline * Rename BreakIntoWords as SplitIntoWords * Remove the double legacy NameTrimmers * Fix compile errors * Temporary: Disable all mods after PrettifyNames * Add SharedPrefix affix config to Vulkan job * Remove unused isContainer param from GetTrimmingName * Work on IdentifySharedPrefixes * Replace regex based NameUtils.LenientUnderscore() with NameSplitter.Underscore() * Fix compile errors due to changes * Remove NonDeterminant/Transformed attribute handling from PrettifyNames * Slightly simplify NameTrimmer * Remove NameTrimmer usage from ExtractNestedTyping and slightly optimize the foreach loops * Slightly cleanup NameTrimmer * Annotate the param names for NameTrimmer's GetPrefix call for readability * Implement visitor for IdentifySharedPrefixes * Properly store scopes * Handle filtering of non C-prefixed names * Work on migrating the NameTrimmer code over to IdentifySharedPrefixes * Cleanup * Add affix handling to IdentifySharedPrefixes * Implement shared prefix identification output to name affixes * Fix incorrect Rewriter implementation * Simplify code * Add missing ModConfiguration attribute * Fix incorrect method call * Fix incorrect member name output for identified prefixes * Fix duplicate key exception caused by functions being able to have duplicate names * Revert "Temporary: Disable all mods after PrettifyNames" This reverts commit 3ec90a7. * Update generator config for OpenAL, OpenGL, and SDL to use IdentifySharedPrefixes * Add another test case as an example of how "2D" is split * Add another prefix override for SeparableTargetEXT This is because 2D isn't trimmed properly by my new name splitter, but I decided that this case was not worth handling in favor of keeping the trimming code simple. See my thoughts here: https://discord.com/channels/521092042781229087/1485943448069738608/1489457083857506336 * Migrate over most prefix trimming/identification tests from PrettifyNamesTests to IdentifySharedPrefixesTests * Update PrettifyNamesTests now that prefix identification is done separately * Migrate over the NameTrimmerTests * Remove IdentifiesSharedPrefix_WhenPrefixesDeclared See #2557 for full reasoning, but the short is that this edge case is purely theoretical and can be handled later on when actually necessary. * Also remove the snapshot for IdentifiesSharedPrefix_WhenPrefixesDeclared * Change INameProcessor to be private to PrettifyNames * Change nPasses to be a class constant to allow usage of doc comments * Rename _forbiddenTrimmings to _forbiddenPrefixes and adjust docs * Change GetPrefix to use a non-null scope parameter for consistency * Add comment explaining why the topological sort exists * Begin replacing the "container" terminology with "scope" * Fix failing tests since the global scope is now represented with an empty string instead of null * Cleanup IdentifySharedPrefixes * Combine EnumInProgress with TypeInProgress * Simplify affix data storage and retrieval * Update docs for Visitor.AffixData * Edit docs for IdentifySharedPrefixes * Swap parameter order on ReportName in IdentifySharedPrefixes * Rewrite PrettifyNames.Visitor to match IdentifySharedPrefixes.Visitor This reduces the amount of code by ~100 lines (1331 -> 1227 lines), which is pretty significant. * Reorder PrettifyNames.Configuration members to improve scannability * Begin rewriting the name processing part of PrettifyNames.ExecuteAsync * Adjust error message for cycle detection * Fix missing method declaration data * Fix missing name data * Add todos and work on updating docs * Change code and comments to refer to trimming names as original names (and some other cleanup) * Move CandidateNames into PrettifyNames * Fix typo * Rename DiscrimStr to GetMethodDiscriminator * Cleanup naming * Use if statements instead of ternaries for populating dictionaries If statements are generally easier to read and debug for this. That said, I do find the original ternary approach better for the method discriminator tuple case, but I decided to use it for all 3 cases for consistency. * Update name conflict resolution code to account for non-method and method names being mixed together now * Add another todo * Change NameProcessorContext to be a class * Begin exposing the entire set of names to name processors and rewriting the rest of the pipeline as INameProcessors * Roughly outline the final implementation and move code to where it roughly belongs * Define name processor inputs * Also provide affix config to ReapplyAffixesProcessor * Implement HandleOverridesProcessor * Cleanup NameProcessorContext * Remove helper data retrieval methods from ScrapedNameData since it can lead to inefficient data access * Remove ScrapedNameData since it's now just an empty wrapper class * Reimplement StripAffixesProcessor * Reimplement PrettifyProcessor * Reimplement ReapplyAffixesProcessor * Reimplement PrefixIfStartsWithNumberProcessor * Edit documentation and add RemoveUnmodifiedFinalNamesProcessor * Add docs for PrefixIfStartsWithNumberProcessor * Implement ResolveConflictsProcessor using adapted name conflict resolution code * Add remark doc comment on ResolveConflictsProcessor about it copying names from the final to working set * Add NameSymbolVisitor for gathering symbols to rename * Add remark comment on nested type support in PrettifyNames * Fix constructors not getting renamed * Add PrettifyNamesTests.ConflictsAreResolved_ForMethodsAndConstants (currently fails) * Work on non-method/method conflict resolution * Also add test case: ConflictsAreResolved_ForMethodsAndConstants_WithAdditionalDiscriminatorAffixes * Ensure conflicts are iterated deterministically This wasn't an actual observed issue, but is a possible edge case I noticed when I was reading the code. The underlying data collection is a hashset or dictionary, both of which have non-deterministic iteration order. This commit adds a tie-breaker to prevent this issue from possibly happening. * Add ConflictsAreResolved_ForMethodsAndConstants_WithAdditionalDiscriminatorAffixes_ReversedPriority and add more comments on why the tests exist * Implement non-method/method conflict resolution and fix secondaries not getting chosen properly * Ensure that the temporary data collections are allocated once and edit comments/variable names to be clearer * Implement "-Value" suffixing for non-methods conflicting with methods and update test cases to reflect expected behavior * Decide it's not worth implementing cross-scope name referencing for affixes yet * Remove unused RenamedType struct and unused MemberData.Name property * Fix -Value suffixing not working properly I expected this to be an issue *eventually*, but apparently it also affects the SDL output already. This is probably because SDL has multiple overloads of the SDL_main method, which my test case does not test for. * Update test snapshot This new snapshot matches the expected output that I wrote in the comments earlier. I just forgot to update the snapshot. * Also resolve referenced affixes from the final set of names and the OpenAL overrides that are now unnecessary * Add ReapplyAffixesProcessor.TryResolveName method (refactor) * Change ReapplyAffixesProcessor to use one global topological sort and update test to indicate that one level of scoping is now supported * Fix issue where overrides break the topological sort * Reorder tests in file * Add test case: SuccessfullyUsesReferencedAffixes_WhenOverridden * Rename NameProcessorContext/NameDataVisitor.Scopes to Names * Work on refactoring name affix processors * Add local OutputName function * Implement the option to prettify individual affixes * Update generator config now that affixes are prettified by default * Don't prettify referenced affixes by default since we usually want the verbatim value of the referenced name * Change affixes to not be prettified by default since it simplifies the code and config * Empty commit * Update snapshot to match expected stated by comment * Adjust doc comment wording for IdentifySharedPrefixes._passCount
github-project-automation
bot
moved this from Todo
to Done
in Silk.NET Development Tracker
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of the PR
This PR chains off of #2555 and rewrites the
NameTrimmerclass as a newIdentifySharedPrefixesmod with equivalent functionality. The goal is to keep all outputs the same while removing edge cases and generally just making the code easier to understand.Additionally, with the trimming code moved out of PrettifyNames, this PR takes advantage of this and simplifies PrettifyNames to an extent (the rest of the refactoring happens in #2564)
Note: Part of the reason this PR chains off of #2555 is because #2555 actually needs some of the changes here to fully work correctly. #2555 can be merged as is, but one unit test has to be commented out and a non-critical feature will be missing.
Related issues, Discord discussions, or proposals
This is the conversation in January when this change was briefly discussed: https://discord.com/channels/521092042781229087/587346162802229298/1457864604951777441
Further Comments
Tasks
LenientUnderscorewithNameSplitter-based implementation2Dare split/underscored.LenientUnderscoreoutputs2D, butNameSplitteroutputs2_Din favor of avoiding any special cases. See my reasoning here: https://discord.com/channels/521092042781229087/1485943448069738608/1489457083857506336IdentifySharedPrefixesmod that identifies shared prefixes and uses the name affix system to annotate those prefixes.Ideally rewrite to avoid the "3 pass" logic. I have no idea how it works still.NameSplittercode for consistency (especially since we have thorough test coverage for this code)NameTrimmertestsPrettifyNamesTests. Since the newIdentifySharedPrefixesmod ended up being an almost direct copy of the core trimming code fromNameTrimmer, I decided to not add too many new tests.IdentifiesSharedPrefix_WhenPrefixesDeclared.GLas a prefix forGL_PIXEL_COUNT_NVandGL_PIXEL_COUNT_AVAILABLE_NVto hide it from the shared prefix identification process._PIXEL_COUNT_NVand_PIXEL_COUNT_AVAILABLE_NV._being the shared prefix. This is definitely a bug since I expect_PIXEL_COUNTorPIXEL_COUNT.GLandPIXEL_COUNTshould be the final affixes.GLPIXEL_COUNT, which is definitely not great behavior.NameSplitter.INameProcessorto be an interface private toPrettifyNamesRegenerate all bindings using Windows and ensure no changes to output