[PATCH] D56424: [clang-tidy] Add check for underscores in googletest names.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:23
+
+constexpr char kDisabledTestPrefix[] = "DISABLED_";
+
----------------
Please use `llvm::StringLiteral`


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:45
+                    const MacroArgs *Args) override {
+    auto IdentifierInfo = MacroNameToken.getIdentifierInfo();
+    if (!IdentifierInfo) {
----------------
please dont use `auto` here, subjectiv as `IdentifierInfo` is the type, but new contributors might not know that, so i would prefer not-auto here.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:49
+    }
+    const StringRef MacroName = IdentifierInfo->getName();
+    if (!isGoogletestTestMacro(MacroName)) {
----------------
no `const` in this case, as its a value and those are not const'ed in LLVM.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:50
+    const StringRef MacroName = IdentifierInfo->getName();
+    if (!isGoogletestTestMacro(MacroName)) {
+      return;
----------------
You can ellide the braces and merge these two ifs.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:58
+    const Token *TestNameToken = Args->getUnexpArgument(1);
+    if (!TestCaseNameToken || !TestNameToken) {
+      return;
----------------
you can ellide the braces


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:62
+    std::string TestCaseName = PP->getSpelling(*TestCaseNameToken);
+    if (TestCaseName.find('_') != std::string::npos) {
+      Check->diag(TestCaseNameToken->getLocation(),
----------------
Maybe the duplicated `diag` call can be merged, you can store the diagnostic with `auto Diag = Check->diag(...)` and pass in the right location and arguments.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+      Check->diag(TestCaseNameToken->getLocation(),
+                  "avoid using \"_\" in test case name \"%0\" according to "
+                  "Googletest FAQ")
----------------
Duplicated message text, you can store it in a local variable/StringRef/StringLiteral,

ellide braces


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:69
+
+    std::string TestName_maybe_disabled = PP->getSpelling(*TestNameToken);
+    StringRef TestName = TestName_maybe_disabled;
----------------
Please use camel-case.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424