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

Tue Apr 5 06:11:13 PDT 2022

erichkeane added a comment.

In D122954#3427676 <https://reviews.llvm.org/D122954#3427676>, @tahonermann wrote:

>> 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.

The only relationship between the two from the web interface is that you tell me that, or I go through other links.  If I have to swap tabs between tests and code, I lose context, or they get separated.

>> 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?

DURING review the addition of tests that the thing changes/fixes being in the same review make the mental load less.

>> 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.

This XFAIL mechanism is very rarely used in the CFE for this purpose.  It is usually more to disable flakey tests or tests that were broken during another patch (or even broken thanks to the but not yet fixed. No matter where you've used this approach before, your strategy here is both not common in the Clang community, AND you have your two reviewers asking you politely to stop.  Frankly, 'review presentation' is for the comfort of the reviewer, not the submitter.

>> 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 one is actually NOT all that self contained (it disables a ton of otherwise functioning tests), and D122958 <https://reviews.llvm.org/D122958> doesn't include addition lines of the tests that it affects.  It actually enables a bunch of tests that worked/behaved correctly even before it. The association between them is vague/unclear.

>> 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.

As someone who has done a TON of code archeology, your strategy here makes it significantly harder in my experience. Which, in part, is why I'm so against it.  From my perspective and experience, this strategy makes Reviews HARDER and archeology HARDER with no decernable benefit to me.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122954/new/

https://reviews.llvm.org/D122954