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

[Kar Epker via Phabricator via cfe-commits]

karepker added a comment.

In D56424#1360318 <https://reviews.llvm.org/D56424#1360318>, @MyDeveloperDay wrote:

> In D56424#1359227 <https://reviews.llvm.org/D56424#1359227>, @karepker wrote:
>
> > In D56424#1357484 <https://reviews.llvm.org/D56424#1357484>, @MyDeveloperDay wrote:
> >
> > > In D56424#1357481 <https://reviews.llvm.org/D56424#1357481>, @lebedev.ri wrote:
> > >
> > > > In D56424#1357471 <https://reviews.llvm.org/D56424#1357471>, @MyDeveloperDay wrote:
> > > >
> > > > > In D56424#1356959 <https://reviews.llvm.org/D56424#1356959>, @karepker wrote:
> > > > >
> > > > > > Hi all, ping on this patch. I've addressed all comments to the best of my ability. Is there anything outstanding that needs to be changed?
> > > > >
> > > > >
> > > > > Round about this time of a review we normally hear @JonasToth asking if you've run this on a large C++ code base like LLVM (with fix-its), and seen if the project still builds afterwards..might be worth doing that ahead of time if you haven't done so already
> > > >
> > > >
> > > > From docs: `This check does not propose any fixes.`.
> > >
> > >
> > > Thats a great suggestion for the future then....     transform
> > >
> > >   TEST(TestCase_Name, Test_Name) {} 
> > >
> > >
> > > into
> > >
> > >   TEST(TestCaseName, TestName) {}
> > >
> >
> >
> > I considered doing this for the check, but decided against it based on the cases in which I've seen underscores in use. I've seen a few cases in the style of this:
> >
> > SuccessfullyWrites_InfiniteDeadline
> > SuccessfullyWrites_DefaultDeadline
> >
> > changing these to:
> >
> > SuccessfullyWritesInfiniteDeadline
> > SuccessfullyWritesDefaultDeadline
> >
> > has a subtle difference to the reader. In the first case, underscore functions like "with", whereas in the fix it sounds like the test is for writing the deadline.
> >
> > While removing the underscore does seem to work for a large portion of the cases, based on the cases like that above, I didn't think it was appropriate to make these modifications.
> >
> > Please let me know what you think.
>
>
> Did I misunderstand I thought the point of the checker was to say "_" in the name was illegal? surely the fixit is at liberty to resolve that?


That's correct, underscore is illegal, but my understanding is that fixits should only be applied if they are able to keep the semantic meaning of the code consistent. While removing the underscore does seem to work in many cases, in some cases, like the one I pointed out above, it does not. Since it's not easy to slice off which cases the fixit is appropriate vs. which cases it isn't, I've opted not to implement the fixit and rely on the developer to fix the name of the test manually.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424