dblaikie added a comment. In D90714#2376667 <https://reviews.llvm.org/D90714#2376667>, @rnk wrote:
> lgtm > > I think the test case looks good as is, FWIW. Token pasting might make it > more readable, but it's improving the existing test, and not necessarily in > scope for this patch. Oh, sorry, didn't see that the big long contiguous identifiers were existing testing. I'm even more inclined towards suggesting the new testing is not the way I'd prefer to go - especially in comparison to the existing testing. The new testing seems to really obscure that the length of the identifier is the important property, by adding a bunch of other features (templates, default template arguments, etc, etc) to the test that don't impact the logic under test - certainly for me that makes it quite a bit less clear what the goal of the test is, and so how to read and maintain it in the future. (but yeah, sorry, didn't mean to suggest the token pasting cleanup as part of this test - I'd misread the delta - but if token pasting makes it easier to add a new test that could be consistent with (after cleaning up the old test - before or after this commit) other tests here, it seems relevant in that regard) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90714/new/ https://reviews.llvm.org/D90714 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
