This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix readability-identifer-naming Hungarian CString options
ClosedPublic

Authored by amurzeau on Feb 20 2023, 2:00 PM.

Details

Summary

When reading readability-identifier-naming.HungarianNotation.CString
options, correctly use the type string stored in CStr.second instead of
the option name (CStr.first) as the HNOption.CString map key.

This will make CString options really working and properly parsed by the
checker.

Diff Detail

Event Timeline

amurzeau created this revision.Feb 20 2023, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2023, 2:00 PM
Herald added a subscriber: xazax.hun. · View Herald Transcript
amurzeau requested review of this revision.Feb 20 2023, 2:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2023, 2:00 PM

Please document fix in the Release Notes, as people might have adopted to the buggy behavior and should know they need to change to the proper one.

clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/identifier-naming/hungarian-notation2/.clang-tidy
118–125

These changes generate a lot of review noise and don't seem to be related to the fix in the patch (which has to do with HungarianNotation.General. Please revert them and if wanted make another patch just for that.

amurzeau updated this revision to Diff 499235.Feb 21 2023, 10:46 AM
amurzeau edited the summary of this revision. (Show Details)

Remove test changes, I will make a different patch for that.

amurzeau updated this revision to Diff 499239.Feb 21 2023, 10:59 AM

Add a release note for this fix.

carlosgalvezp added inline comments.Feb 21 2023, 11:27 AM
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/identifier-naming/hungarian-notation2/.clang-tidy
115

Sorry, maybe I wasn't clear. Isn't this line meant to stay, since this is what the patch is fixing?

amurzeau marked an inline comment as done.Feb 22 2023, 11:30 AM
amurzeau added inline comments.
clang-tools-extra/test/clang-tidy/checkers/readability/Inputs/identifier-naming/hungarian-notation2/.clang-tidy
115

The patch only fixes reading readability-identifier-naming.HungarianNotation.CString.* options.

I've updated the test completely to make options parsing issues tested.
The test was not catching the issue with HungarianNotation.CString.* because all options were set to their default value in this .clang-tidy file, so the test couldn't really check that the option was really correctly read (and it turns out it wasn't for HungarianNotation.CString.*).

This one (readability-identifier-naming.HungarianNotation.General.TreatStructAsClass) is correctly read, but was just misnamed in this test file with Options instead of General.
I found that while updating this test.

Both the code and docs currently use readability-identifier-naming.HungarianNotation.General.TreatStructAsClass.

The minimal changes in this test that reproduce the exact issue that is fixed here are the changes on readability-identifier-naming.HungarianNotation.CString.* options.
But I found it weird to just test them without updating all other options as well, that's why I've updated all options instead.

carlosgalvezp accepted this revision.Feb 22 2023, 11:47 PM

Alright, sounds good, thanks for the clarification! Then I misunderstood the change in the tests and indeed it belonged together in this patch, sorry for the confusion. I have approved both patches so we can land them now. Can you land the patch or would you like me to do it for you? If so please provide name and email (same as your Github account) for attribution.

This revision is now accepted and ready to land.Feb 22 2023, 11:47 PM

Alright, sounds good, thanks for the clarification! Then I misunderstood the change in the tests and indeed it belonged together in this patch, sorry for the confusion. I have approved both patches so we can land them now. Can you land the patch or would you like me to do it for you? If so please provide name and email (same as your Github account) for attribution.

No worries, yes please land the patches for me as Alexis Murzeau <amubtdx@gmail.com> (phabricator mail and handle is the same as github).
Thanks!