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

Mon Apr 4 13:23:04 PDT 2022

aaron.ballman added a comment.

In D122954#3427422 <https://reviews.llvm.org/D122954#3427422>, @erichkeane wrote:

> In D122954#3427406 <https://reviews.llvm.org/D122954#3427406>, @tahonermann wrote:
>
>>> FWIW, I dislike this idea of doing tests in separate commits from the patch itself, it makes the review of the patch more difficult, and makes looking through history more difficult.
>>
>> Here is my perspective on this. When adding a new feature, I agree that including tests with the code is useful (the tests usually have no meaningful behavior without the new code in that case). I think the motivation is different for bug fixes though, particularly when there is an existing testing gap. Had these tests been added with the initial `target_clones` work, then the bug fix commit would reveal the precise change in behavior that is the result of the bug fix. I think that is quite valuable both as a review tool and as part of the code history. By adding the test in a separate commit from the fix, it is possible to observe both the previous undesirable behavior as well as precisely what changed with the fix. This also helps to enable someone that is looking to identify a commit responsible for a bug fix (e.g, someone that is looking to resolve a bug report as works-for-me-in-release-X) to definitively identify a commit as exhibiting the expected fix.
>>
>> I've practiced this approach at other places I've worked and never found it to make the review process more difficult. If you can elaborate on why you feel it makes review more challenging, perhaps there is another way to address that.
>
> I understand it and just disagree. I've NEVER had a tough time 'seeing the previous behavior' vs 'the current behavior', but I DO have the opposite problem: Figuring out what the associated tests are for a patch.
>
> 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.

Strong +1 to this. As a reviewer, 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. 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. 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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122954