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

[Erich Keane via Phabricator via cfe-commits]

erichkeane requested changes to this revision.
erichkeane added a comment.
This revision now requires changes to proceed.

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.

Additionally, and I just noticed, 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 think this is unacceptable enough for me to 'request changes'.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122954