karepker added a comment. 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. 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
