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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang-tidy/google/AvoidUnderscoreInGoogletestNameCheck.cpp:64
+      Check->diag(TestCaseNameToken->getLocation(),
+                  "avoid using \"_\" in test case name \"%0\" according to "
+                  "Googletest FAQ")
----------------
MyDeveloperDay wrote:
> karepker wrote:
> > 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.
> I think it would be enough to have one diag message saying 
> 
> ```
> diag(Loc,"don't use "_" in %s() ",testMacro) 
> ```
> 
> simply elide the name of the parameter, the user will be drawn to the line and will see they are using an "_" in either testname or testcase, I don't think it necessary to tell them which parameter is incorrect..
> 
> And so whilst I see why you might prefer to generate 2 different messages for either testcase and testname, isn't it also true that these messages will be wrong if your using one of the other macros likes TEST_F() shouldn't the second argument now be testfixture? and if I'm using TEST_P() shouldn't it be pattern or parameterized?
> 
> This is why I think the test macros used would be useful as an option, by generalizing the checker by removing the "googletest" specifics makes the checker much more useful.
> 
> I know for one, I don't use gtest, but a similar framework (with the same issues), and I'd use your checker if I could customize it.
> 
> 
> 
> 
minor correction. to my comment

```
diag(Loc,"avoid using \"_\" in %s() ",testMacro)
```


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424