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

Tue Apr 5 04:47:48 PDT 2022

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

I disagree. If I need to roll back D122958 <https://reviews.llvm.org/D122958>, that means I also need to roll back other changes in the stack because they rely on the ones in the first patch. Alternatively, if the changes aren't related, then use of a patch stack is perhaps not appropriate (then the reviews should be independent, not interdependent).

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

Again, I disagree. This separation means when I do a `git blame`, I'll get a commit somewhere in the patch stack. Then I have to go look at the review to find out there were related commits. (This is not hypothetical -- I've run into this frustration a few times in recent history and each time it's been either a patch stack or a follow-up commit due to built bot breakage.) It's a minor annoyance because I can still find the information I need to find, but it's worth keeping in mind.

This may be a cultural thing, but personally, I find patch stacks to be really difficult to review when they're as split apart as this one has been. It's not that your approach isn't defensible -- you've obviously put thought into this! For me, what makes it hard is that I read the code and I go "where's the test associated with this change" or "I wonder if they thought about this edge case", and with patch stacks like this, that involves an easter egg hunt because your thought process to get to the final state may take different paths than mine. And when I am looking at a review in the "middle" of the patch stack, like this one, it gets even harder. In order to understand the context for these test changes, I have to go find four other things first and keep *all* of that context in mind. Eventually, what I wind up doing is opening up a browser tab for each patch in the stack and tab between them when I need context. It's not ideal, but it gets the job done.

In this particular case, the start of the patch stack has functional changes, followed by three NFC changes, followed by this change to test cases. If the previous three patches truly are NFC, then there's no reason why this change to just the tests shouldn't be rolled into the original functional change *or* be rolled into the next patch that has functional changes intended to fix this test. (From looking at the patch stack, I'd have expected 2-3 reviews, not 7)

This isn't to admonish or discourage you from using patch stacks to help split logical things up, but it is to caution against splitting things too much because it does add a cognitive burden to me as a reviewer. Also, other reviewers and contributors may have different feelings on the matter. We don't have strict rules in this area because we want some flexibility with people's workflows (both contributors and reviewers).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122954