[3.0] Heavily expand testing suite; Rewrite NameUtils.Prettify; Fix a lot of subtle naming bugs #28
Conversation
…r to trim the primaries instead of the original name
…mit the test that already passes
…uffixes_AndShortenedNamesConflict
…rom the jobData not the options snapshot
…dCandidateNames; Replace the dictionary used with a list since it was a glorified list of tuples I might change it back to be a dictionary later, but using a list is safer for now since it ensures that all accesses need to directly access a property of AugmentedCandidateNames. This change was originally to fix an issue where trimming names collide after affixes are removed. This is an issue introduced by my change to the INameTrimmer order (see f33d592) since I made it so that affixes are now removed before NameTrimmer runs.
…rSuffixes_AndShortenedNamesConflict
…se with PrettifyNames_TrimsSharedPrefix2
…TypeName_WhenNotMatchingHint_AndOnlyOneType
…ffectSharedPrefixTrimming_WhenAffixesDeclared
…edPrefixTrimming_WhenAffixesDeclared
Exanite
left a comment
Exanite
left a comment
There was a problem hiding this comment.
Partially reviewed, but submitting now so that pending comments are visible.
| --exclude | ||
| _GUID | ||
| AL_CPLUSPLUS | ||
| ALC_CPLUSPLUS |
There was a problem hiding this comment.
These aren't useful since they just provide the C++ version.
| @@ -1,2 +1,3 @@ | |||
| @../../remap-stdint.rsp | |||
There was a problem hiding this comment.
These make generation between Windows/Linux consistent, similar to how it's being used in the Vulkan bindings.
| GLclampd=double | ||
| GLhandleARB=uint | ||
| GLintptrARB=nint | ||
| GLsizeiptrARB=nuint |
There was a problem hiding this comment.
The non-ARB versions were already remapped.
This also makes the bindings generate consistently between Linux/Windows.
| /// capitals word. | ||
| /// </summary> | ||
| public int? LongAcronymThreshold { get; init; } | ||
| public int LongAcronymThreshold { get; init; } = 3; // TODO: Change default to 2 in next PR to match framework design guidelines |
There was a problem hiding this comment.
Cleanup to make the default be specified in the config instead of later.
This is easier to understand and prevents inconsistencies.
There was a problem hiding this comment.
This was only used by NameTrimmer so it has been moved there (and also renamed to AugmentedCandidateNames).
| var prefix = | ||
| string prefix; | ||
| if ( | ||
| container is not null | ||
| && (prefixOverrides?.TryGetValue(container, out var @override) ?? false) | ||
| ? @override | ||
| : names.Count == 1 && !string.IsNullOrWhiteSpace(containerTrimmingName) | ||
| ? NameUtils.FindCommonPrefix( | ||
| [ | ||
| names.Keys.First(x => !(nonDeterminant?.Contains(x) ?? false)), | ||
| containerTrimmingName, | ||
| ], | ||
| true, |
There was a problem hiding this comment.
Big diff, but only two changes:
- The nested ternary was replaced with if statements. Ternaries are horrible to step through using a debugger.
- I added the "One name. Can't determine prefix." case to fix
DoesNotTrimTypeName_WhenNotMatchingHint_AndOnlyOneType(OcclusionQueryParameterNameNVwas being trimmed asQueryParameterNameNVONLY during test cases since the test cases usually only have one type)
| return $"{hint}_{name[hint.Length..].Trim('_').LenientUnderscore()}"; | ||
| } | ||
|
|
||
| return name.Trim('_').LenientUnderscore(); |
There was a problem hiding this comment.
Replaced ternary with ifs.
Added trim to what was originally the else branch to fix a bug related to the behavior of this code:
The bug caused prefixes to not be trimmed.
| [Test] | ||
| public void RegressionNamespacePrefixDetermination() => | ||
| Assert.That( | ||
| NameUtils.FindCommonPrefix(["Silk.NET.SDL", "Silk.NET.SDL"], true, false, true), |
| ); | ||
|
|
||
| [Test] | ||
| public void OverzealousNameTrimming() |
There was a problem hiding this comment.
OverzealousNameTrimming has been removed since the behavior it tests for no longer exists. It has been replaced with 3 tests, but many more also test for similar behavior:
| } | ||
|
|
||
| [Test] | ||
| public void OverzealousNameTrimmingFixupIsNotOverzealousForOpenAL() |
There was a problem hiding this comment.
This has been replaced by PrettifyNamesTests.TrimsSharedPrefix2.
Perksey
left a comment
Perksey
left a comment
There was a problem hiding this comment.
I'm happy notwithstanding the below comment
sources/SDL/SDL/SDL3/Sdl.gen.cs
Outdated
| [NativeFunction("SDL3", EntryPoint = "SDL_LoadWAV_IO")] | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization)] | ||
| public static byte LoadWAVIO( | ||
| public static byte LoadWavIo( |
There was a problem hiding this comment.
This is an interesting test case, should this be LoadWavIO?
tests/SilkTouch/SilkTouch/Naming/PrettifyNamesTests.TrimsSharedPrefix_ForTypes.verified.txt
Outdated
Show resolved
Hide resolved
These changes are expected because the default acronym threshold is now 2 and the suffixes are not declared as NameAffixes.
[3.0] Change acronym threshold to 2 for all bindings
Summary of the PR
This PR contains all changes from #26 and #27, and is designed to replace those two PRs.
Reasoning is that #27 modifies a lot of code from #26 so it is easier to review the two sets of changes as a single set.
Note: All important information from the two above PRs are also present here.
This PR does the following:
OverzealousNameTrimmingtest case with 3 new test casesPrettifyNamesTests.TrimsSharedPrefixPrettifyNamesTests.TrimsSharedPrefix_WhenAffixesDeclaredMixKhronosDataTests.IdentifiesVendorSuffixesNameUtils.PrettifywithNamePrettifier.PrettifyNameUtils.Prettifymethod is that it was very difficult to modify and understand due to the use of regexes and Humanizer's utilities. The newNamePrettifierreplaces it and is written in plain C# with the goal of being consistent, easy to understand and modify, and fully covered by tests.NameSplitterclass for splitting names into multiple parts.NamePrettifier. I'm considering using this forNameTrimmerso that the codebase is consistent and doesn't rely on Humanizer/regexes (eg:LenientUnderscore) for splitting.Related issues, Discord discussions, or proposals
Discord discussion on
OverzealousNameTrimming: https://discord.com/channels/521092042781229087/587346162802229298/1458562017475432480Discord thread for this PR: https://discord.com/channels/521092042781229087/1459408120748183643
Further Comments
Todo list:
OcclusionQueryParameterNameNVis currently being trimmed asQueryParameterNameNVwhen it is the only type being trimmedNameTrimmerjust trims the first word in this caseglas a prefix hint. This might be related. I need to investigate.NameTrimmer)? Before stripping affixes or after?NameTrimmer.NameAffixes will be suffixes.NameTrimmerafter stripping affixes, the effect is less of the name is trimmed, which should fix a few cases whereNameTrimmertrims too much, which can cause affix stripping to output""or"_", which sub-sequentially errors during prettification.NameTrimmer)OverzealousNameTrimmingFixupIsNotOverzealousForOpenALtest case. It overlaps with my new tests, but doesn't hurt. The name might be misleading though.