tahonermann added a comment. > but I DO have the opposite problem: Figuring out what the associated tests > are for a patch
I also have that issue, but I don't see the relevance here. The changes in D122958 <https://reviews.llvm.org/D122958> that fixes the issues revealed by these tests includes the test updates. So that commit reveals exactly which tests are relevant. > It loses my ability to make sure that your patch covers all of the cases that > are important, and it loses my ability to figure out what the patch is doing > from the test itself. I'm not following here. There are two distinct issues; a testing gap that is addressed by this review, and a code issue that is addressed by D122958 <https://reviews.llvm.org/D122958>. Addressing these separately is good separation of concerns. Should there be additional tests of declarations that are not definitions? If so, that would be appropriate to discuss in this review? > this actually disables a LOT of other tests. So in the 'meantime' between the > two patches, we are actually losing test coverage thanks to the 'XFAIL'. I do see `XFAIL` used for such temporary purposes based on a quick review of `git log`. See commit 9001168cf8b8c85ec9af9b91756b39d2da0130bf <https://reviews.llvm.org/rG9001168cf8b8c85ec9af9b91756b39d2da0130bf> for example. I've used this approach elsewhere for many years. > I want the patch to be making one logical change so that it remains > manageable to review, but it needs to be completely self-contained > (functional changes, docs, tests, et al) even if that makes the patch a bit > bigger. I strongly agree; that is exactly why these commits are split as they are. Both this review and D122958 <https://reviews.llvm.org/D122958> are self-contained. > This is especially important because it makes it far easier to revert > problematic commits -- if the functional change has an issue, having to > separately revert several other related commits is a burden. I agree that would be a burden, but it isn't the case here. > It also makes git blame more useful when doing code archeology; you can git > blame a test to get to the functional changes more quickly. That is exactly what this separation enables. `git blame` on the tests will get you both the change for the testing gap as well as the functional change that fixes the behavior. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122954/new/ https://reviews.llvm.org/D122954 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
