[PATCH] D115634: [clangd] Cleanup of readability-identifier-naming

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

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.



================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:103
 
-static clang::clangd::Key<std::string> kFileBeingProcessed;
+static clang::clangd::Key<std::string> KFileBeingProcessed;
 
----------------
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.


================
Comment at: clang-tools-extra/clangd/fuzzer/FuzzerClangdMain.cpp:15
 
+// NOLINTNEXTLINE(readability-identifier-naming)
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size);
----------------
This name is always exempt, and should rather be excluded by config: https://clang.llvm.org/extra/clang-tidy/checks/readability-identifier-naming.html#cmdoption-arg-functionignoredregexp


================
Comment at: clang-tools-extra/clangd/index/YAMLSerialization.cpp:176
 template <> struct MappingTraits<SymbolInfo> {
-  static void mapping(IO &io, SymbolInfo &SymInfo) {
+  static void mapping(IO &Io, SymbolInfo &SymInfo) {
     // FIXME: expose other fields?
----------------
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)


================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:39
 ::testing::Matcher<const RefSlab &>
-RefsAre(std::vector<::testing::Matcher<Ref>> Matchers) {
+refsAre(std::vector<::testing::Matcher<Ref>> Matchers) {
   return ElementsAre(::testing::Pair(_, UnorderedElementsAreArray(Matchers)));
----------------
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_P`s
- 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).


================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:228
   EXPECT_THAT(getRefs(Idx, Common.ID),
-              RefsAre({FileURI("unittest:///root/A.h"),
+              refsAre({FileURI("unittest:///root/A.h"),
                        FileURI("unittest:///root/A.cc"),
----------------
this illustrates the problem: `RefsAre({FileURI(...)})` or `refsAre({fileURI})` make sense, but the mix does not.


================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:272
       )cpp";
-  std::string A_CC = "";
+  std::string ACc = "";
   FS.Files[testPath("root/A.cc")] = R"cpp(
----------------
This new name is no good, but in fact the variable is unused, just delete it


================
Comment at: clang-tools-extra/clangd/unittests/BackgroundIndexTests.cpp:346
       )cpp";
-  std::string A_CC = "#include \"A.h\"\nvoid g() { (void)common; }";
-  FS.Files[testPath("root/A.cc")] = A_CC;
+  std::string ACc = "#include \"A.h\"\nvoid g() { (void)common; }";
+  FS.Files[testPath("root/A.cc")] = ACc;
----------------
This name is also no good, and it's used exactly once - inline it


================
Comment at: clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp:25
 namespace config {
-template <typename T> void PrintTo(const Located<T> &V, std::ostream *OS) {
+template <typename T> void printTo(const Located<T> &V, std::ostream *OS) {
   *OS << ::testing::PrintToString(*V);
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115634/new/

https://reviews.llvm.org/D115634