Page MenuHomePhabricator

[clang-apply-replacements] Added an option to ignore insert conflict.
ClosedPublic

Authored by Sockke on Apr 18 2022, 12:56 AM.

Details

Summary

If two different texts are inserted at the same offset, clang-apply-replacements prints the conflict error and discards all fixes. This patch adds support for adjusting conflict offset and keeps running to continually fix them.

https://godbolt.org/z/P938EGoxj doesn't have any fixes when I run run-clang-tidy.py to generate a YAML file with clang-tidy and fix them with clang-apply-replacements. The YAML file has two different header texts insertions at the same offset, unlike clang-tidy with '-fix', clang-apply-replacements does not adjust for this conflict.

Diff Detail

Event Timeline

Sockke created this revision.Apr 18 2022, 12:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 12:56 AM
Sockke requested review of this revision.Apr 18 2022, 12:56 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 18 2022, 12:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Sockke edited the summary of this revision. (Show Details)Apr 18 2022, 12:59 AM
Enna1 added a subscriber: Enna1.Apr 18 2022, 1:02 AM
Sockke retitled this revision from [clang-apply-replacements] Added support for adjusting conflict offset. to [WIP][clang-apply-replacements] Added support for adjusting conflict offset..Apr 18 2022, 3:34 AM
Sockke updated this revision to Diff 423355.EditedApr 18 2022, 4:29 AM
Sockke retitled this revision from [WIP][clang-apply-replacements] Added support for adjusting conflict offset. to [clang-apply-replacements] Added support for adjusting conflict offset..

Added an option to ignore insert conflict and this option was enabled by default in run-clang-tidy.py.

It stands to reason that there should be no insert conflicts in the YAML generated by clang-tidy except for the header insertion. The two checks that caused the conflict should be fixed in clang-tidy.

Sockke updated this revision to Diff 423359.Apr 18 2022, 5:04 AM
Sockke retitled this revision from [clang-apply-replacements] Added support for adjusting conflict offset. to [clang-apply-replacements] Added an option to ignore insert conflict..
MTC added a subscriber: MTC.Apr 25 2022, 9:01 PM
MTC added inline comments.
clang/include/clang/Tooling/Refactoring/AtomicChange.h
119

IMHO, there is no need to rename the method to get the non-const version. The following code is ok enough

Replacements &getReplacements() { return Replaces; }

Sockke updated this revision to Diff 425166.Apr 26 2022, 3:24 AM

Rename the function.

Sockke marked an inline comment as done.Apr 28 2022, 7:28 AM

Friendly ping.

I don't know enough about this code base to feel comfortable signing off on it, but the changes look reasonable to me FWIW.

MTC added a comment.May 4 2022, 6:51 PM

I don't know enough about this code base to feel comfortable signing off on it, but the changes look reasonable to me FWIW.

Thanks, Aaron! I will find another guy to review the change.

Friendly ping.

Hi, Could anyone please review this diff?

aaron.ballman accepted this revision.May 25 2022, 10:39 AM

I think this has sat for long enough and my LGTM will have to suffice. :-)

This revision is now accepted and ready to land.May 25 2022, 10:39 AM
Sockke updated this revision to Diff 432522.May 27 2022, 4:41 AM

rebased.

Sockke updated this revision to Diff 432829.Sun, May 29, 8:28 PM