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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
