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