Page MenuHomePhabricator

[clangd] Cleanup of readability-identifier-naming
ClosedPublic

Authored by kuhnel on Dec 13 2021, 6:49 AM.

Details

Summary

Auto-generated patch based on clang-tidy readability-identifier-naming.
Only some manual cleanup for extern "C" declarations and a GTest change was required.

I'm not sure if this cleanup is actually very useful. It cleans up clang-tidy findings to the number of warnings from clang-tidy should be lower. Since it was easy to do and required only little cleanup I thought I'd upload it for discussion.

One pattern that keeps recurring: Test matchers are also supposed to start with a lowercase letter as per LLVM convention. However GTest naming convention for matchers start with upper case. I would propose to keep stay consistent with the GTest convention there. However that would imply a lot of //NOLINT throughout these files.

To re-product this patch run:

run-clang-tidy -checks="-*,readability-identifier-naming" -fix -format ./clang-tools-extra/clangd

To convert the macro names, I was using this script with some manual cleanup afterwards:
https://gist.github.com/ChristianKuehnel/a01cc4362b07c58281554ab46235a077

Diff Detail

Event Timeline

kuhnel created this revision.Dec 13 2021, 6:49 AM
kuhnel requested review of this revision.Dec 13 2021, 6:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 13 2021, 6:49 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
kuhnel edited the summary of this revision. (Show Details)Dec 13 2021, 7:06 AM
kuhnel added subscribers: sammccall, hokein.
kuhnel retitled this revision from Cleanup of readability-identifier-naming to [clangd] Cleanup of readability-identifier-naming.Dec 13 2021, 7:38 AM

Generally looks good with a few nits for individual names.

The matcher cleanup is a problem though, because you haven't cleaned up the functions created using the macros MATCHER, MATCHER_P, MATCHER_P2 etc. Presumably this is because names created through macros are excluded by the clang-tidy check.
IMO as a question of style we need to change all such functions we define or none of them.

clang-tools-extra/clangd/TUScheduler.cpp
103

I don't think it makes sense to keep the K here if it has to be uppercase.

The LLVM styleguide doesn't encourage this prefix. Styleguides that do tend to use a lowercase k for readability. If we want to strictly follow the LLVM styleguide I think we should just drop it.

clang-tools-extra/clangd/fuzzer/FuzzerClangdMain.cpp
15 ↗(On Diff #393876)
clang-tools-extra/clangd/index/YAMLSerialization.cpp
164

We use IO rather than Io for initialisms.
There are cases where the type/name can't match, but this doesn't appear to be one of them (we use IO &IO in similar mapping function below)

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
39

Note that MATCHER_P(FileURI, F, "") { ... ] above also defines a function returning a Matcher.

I think it's OK for our functions to have different case than matcher functions from the upstream gtest (different libraries have different styles), but I don't think it's OK for matcher functions to have different case depending on whether MATCHER_P was used or not.

And I agree that //NOLINT everywhere is not a good option.

So as far as I'm concerned:

  • OK: change these functions and also all the MATCHER, MATCHER_P etc instances to lowerCamelCase
  • OK: ignore this style rule and check results for matcher names
  • Not OK: follow the rule for explicitly-defined function but ignore it for MATCHER_Ps
  • Not OK: ignore the rule and add // NOLINT to enough exceptions to silence the checker
  • Bonus: teach the tidy check to flag violations introduced through macros (probably with an allowlist of macro names)

(I'm not quite sure how our convention of using UpperCamelCase for matcher factory functions got started, but I suspect it's from following upstream recipes without thinking hard about whether the new symbol is a function or a type).

228–229

this illustrates the problem: RefsAre({FileURI(...)}) or refsAre({fileURI}) make sense, but the mix does not.

272–273

This new name is no good, but in fact the variable is unused, just delete it

346–347

This name is also no good, and it's used exactly once - inline it

clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
28

This is a magic function used by GTest, it is found via ADL and must be spelled exactly PrintTo.

This rename is dangerous because it doesn't cause tests to fail, but it causes them to have useless error messages if they fail later.

Please exclude this from the checker using the FunctionIgnoredRegexp.
Please make sure no other PrintTo functions have been renamed.

Discussion in team sync, work for myself:

  • most changes are in unittests --> propose something for the matchers
  • cleaning up constants makes sense
  • clean up this patch, then do another round of reviews
kuhnel updated this revision to Diff 403538.Jan 27 2022, 12:55 AM

Re-created the patch as there were too many changes in the repo.

Excluding gtest Matchers from naming checks as gtest convention does not match
LLVM convention.

kuhnel edited the summary of this revision. (Show Details)Jan 27 2022, 1:02 AM
kuhnel marked 6 inline comments as done.
kuhnel added inline comments.
clang-tools-extra/clangd/fuzzer/FuzzerClangdMain.cpp
15 ↗(On Diff #393876)

I tried a couple of variations of the config in the global .clang-tidy file, however none of them actually ignored the function:

yaml
  - key:             readability-identifier-naming.FunctionIgnoredRegexp
    value:           "LLVMFuzzerTest.*"

Any idea what I'm doing wrong?

clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp
39

My proposal here is to stick with the gtest naming convention for more consistency in the naming: Matchers start with upper case letters.

Thus I added //NOLINT to all matchers I encountered.

kuhnel updated this revision to Diff 403539.Jan 27 2022, 1:03 AM
kuhnel marked an inline comment as done.

addressed review comments

kuhnel marked an inline comment as done.Jan 27 2022, 1:04 AM
kuhnel edited the summary of this revision. (Show Details)Feb 1 2022, 4:18 AM
kuhnel updated this revision to Diff 404881.Feb 1 2022, 4:20 AM
kuhnel edited the summary of this revision. (Show Details)

re-created patch, then updated matcher macros

kuhnel updated this revision to Diff 404882.Feb 1 2022, 4:26 AM

fixed A_CC in BackgroundIndexTests.cpp

sammccall accepted this revision.Feb 1 2022, 5:03 AM

Thanks, this is great!
Just a few cases that regex couldn't quite understand, and one bona fide bug!

clang-tools-extra/clangd/TUScheduler.cpp
103

I think this got reverted - should just be FileBeingProcessed

clang-tools-extra/clangd/XRefs.cpp
1864–1866

this was a bug! should be VisitCoyieldExpr (capital because it's a CRTP override).

But this is is a functional bugfix, please either leave a fixme or change in a separate patch

clang-tools-extra/clangd/index/YAMLSerialization.cpp
164

This change also got lost (and in the next function)

clang-tools-extra/clangd/unittests/SerializationTests.cpp
117–118

id, not iD

This revision is now accepted and ready to land.Feb 1 2022, 5:03 AM
kuhnel marked 4 inline comments as done.Feb 1 2022, 5:31 AM

Thx @sammccall for your patience with this cleanup.

clang-tools-extra/clangd/TUScheduler.cpp
103

Right, sorry about the backsliding there. If I were ever to do such a cleanup again, I need to really split manual and automatic steps into separate commits and keep them in a separate branch before squashing them for arc diff.

clang-tools-extra/clangd/XRefs.cpp
1864–1866

added FIXME, feel feel free to create a patch for it

This revision was landed with ongoing or failed builds.Feb 1 2022, 5:32 AM
This revision was automatically updated to reflect the committed changes.
kuhnel marked an inline comment as done.