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

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122954