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

[Kar Epker via Phabricator via cfe-commits]

karepker added a comment.

In D56424#1349336 <https://reviews.llvm.org/D56424#1349336>, @lebedev.ri wrote:

> In D56424#1349218 <https://reviews.llvm.org/D56424#1349218>, @karepker wrote:
>
> > Clean up comments in test file.
>
>
> For reference, what documentation sources did you read when creating the review itseft?
>  I thought it was documented that an appropriate category (`[clang-tidy]`) should be present in the subject.


I see now that putting the category in the commit message is recommend by http://llvm.org/docs/DeveloperPolicy.html#commit-messages, though if I were reading that for the first time, it would have been unclear to me what the protocol for determining the category is (e.g. should this be `[clang-tidy]` vs. `[clang-tools-extra]`)?

When I was preparing this for review, though, I was mostly going by example off this previous patch: https://reviews.llvm.org/D18408.



================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:1
+//===--- AvoidUnderscoreInGoogletestNameCheck.cpp - clang-tidy-------------===//
+//
----------------
MyDeveloperDay wrote:
> nit: space after tidy and before ---
I think I did this. Not sure which "---" you're referring to, but I think this comment is now in line with other clang-tidy comments I've looked at.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:62
+    std::string TestCaseName = PP->getSpelling(*TestCaseNameToken);
+    if (TestCaseName.find('_') != std::string::npos) {
+      Check->diag(TestCaseNameToken->getLocation(),
----------------
JonasToth wrote:
> 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.
I don't think I understand the suggestion. Don't I have to pass in the location immediately? `Check->diag(...)` doesn't seem to have an overload that allows deferring passing in the location until later.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+      Check->diag(TestCaseNameToken->getLocation(),
+                  "avoid using \"_\" in test case name \"%0\" according to "
+                  "Googletest FAQ")
----------------
JonasToth wrote:
> Duplicated message text, you can store it in a local variable/StringRef/StringLiteral,
> 
> ellide braces
The message text is not quite the same. The first says "test case name" and the second says "test name" per the naming conventions for the two name arguments to the test macros in Googletest. I preferred having these as two separate strings instead of trying to add the word "case" conditionally to one, which I thought would be too complex.

Please let me know if you feel differently or have other suggestions regarding the message.

Braces have been removed.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424