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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

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?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D56424