[PATCH] D122954: [clang] Extend target_clones tests to exercise declarations that are not definitions.

Mon Apr 4 14:11:27 PDT 2022

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