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

2022-04-05 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann abandoned this revision. tahonermann added a comment. Abandoning this review; the changes made here have been squashed into D122958 . With regard to prior comments, it seems we have divergent experience regarding the utility of separating commits as

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

2022-04-05 Thread Erich Keane via Phabricator via cfe-commits
erichkeane added a comment. In 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 > D1229

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

2022-04-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In 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 > D1

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

2022-04-04 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann added a comment. > 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 that fixes the issues revealed by these tests includ

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

2022-04-04 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In D122954#3427422 , @erichkeane wrote: > In D122954#3427406 , @tahonermann > wrote: > >>> FWIW, I dislike this idea of doing tests in separate commits from the patch >>> itself,

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

2022-04-04 Thread 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 , @tahonermann wrote: >> FWIW, I dislike this idea of doing tests in separate commits from the patch >> itself,

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

2022-04-04 Thread 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 incl

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

2022-04-04 Thread Erich Keane via Phabricator via cfe-commits
erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. 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.

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

2022-04-02 Thread Tom Honermann via Phabricator via cfe-commits
tahonermann created this revision. tahonermann added reviewers: erichkeane, aaron.ballman. Herald added a project: All. tahonermann added inline comments. tahonermann published this revision for review. Herald added a project: clang. Herald added a subscriber: cfe-commits. Commen