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