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

[Christian Kühnel via Phabricator via cfe-commits]

kuhnel marked 6 inline comments as done.
kuhnel added inline comments.


================
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);
----------------
sammccall wrote:
> 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
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?


================
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)));
----------------
sammccall wrote:
> 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).
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115634