This is an archive of the discontinued LLVM Phabricator instance.

Merge conflicting replacements when they are order-independent.
ClosedPublic

Authored by ioeric on Sep 21 2016, 7:50 AM.

Details

Summary

Now two replacements are considered order-independent if applying them in
either order produces the same result. These include (but not restricted
to) replacements that:

  • don't overlap (being directly adjacent is fine) and
  • are overlapping deletions.
  • are insertions at the same offset and applying them in either order has the same effect, i.e. X + Y = Y + X if one inserts text X and the other inserts text Y.

Discussion about this design can be found in D24717

Diff Detail

Event Timeline

ioeric updated this revision to Diff 72042.Sep 21 2016, 7:50 AM
ioeric retitled this revision from to Merge conflicting replacements when they are order-independent..
ioeric updated this object.
ioeric edited subscribers, added: cfe-commits; removed: klimek.
ioeric updated this revision to Diff 72045.Sep 21 2016, 8:08 AM
  • Provide order-independent replacements examples in comments.
ioeric added a subscriber: omtcyfz.Sep 26 2016, 7:18 AM

Friendly ping :)

klimek added inline comments.Sep 27 2016, 3:55 AM
include/clang/Tooling/Core/Replacement.h
177–178

This seems incorrect. What am I missing?

ioeric added inline comments.Sep 27 2016, 3:58 AM
include/clang/Tooling/Core/Replacement.h
177–178

Here is the discussion about this: https://reviews.llvm.org/D24717

ioeric updated this object.Sep 27 2016, 3:58 AM
klimek added inline comments.Sep 27 2016, 5:00 AM
include/clang/Tooling/Core/Replacement.h
177–178

I misunderstood :) Perhaps change to:
, i.e. if X + Y == Y + X when inserting X and Y respectively.

242

typo: otheriwse

lib/Tooling/Core/Replacement.cpp
162

I'd not use auto for simple integer types.

166

Add comment why we can assert this.

179–181

These names are confusing me...

184

This comes from a single replacement - why do we need to call merge?

ioeric updated this revision to Diff 72642.Sep 27 2016, 6:26 AM
ioeric marked 6 inline comments as done.
  • Addressed review comments.
lib/Tooling/Core/Replacement.cpp
179–181

Trying to make names less confusing... any better now?

184

Because it refers to code after Replaces are applied?

klimek added inline comments.Sep 27 2016, 8:52 AM
lib/Tooling/Core/Replacement.cpp
179–181

I think this is a bit subtle and needs a lot more comments to guide me along what is happening and why ...

ioeric updated this revision to Diff 72666.Sep 27 2016, 9:12 AM
ioeric marked 3 inline comments as done.
  • Addressed review comments.
lib/Tooling/Core/Replacement.cpp
179–181

Ahh, right! Should've done that...

Comments added.

klimek accepted this revision.Sep 28 2016, 12:44 AM
klimek edited edge metadata.

lg

lib/Tooling/Core/Replacement.cpp
179–181

Much better. Thanks.

This revision is now accepted and ready to land.Sep 28 2016, 12:44 AM
This revision was automatically updated to reflect the committed changes.
alexander-shaposhnikov added inline comments.
cfe/trunk/lib/Tooling/Core/Replacement.cpp
182 ↗(On Diff #72799)

sorry, probably i am late to the party,
however i'd like to ask how this method is supposed to be used by the customers of this code.
If i'm not mistaken we have some concrete examples in clang-extra-tools: for instance @omtcyfz referred to this diff on https://reviews.llvm.org/D24914.
Basically after this diff the class Replacements contains several methods (add, mergeIfOrderIndependent etc), but mergeIfOrderIndependent potentially will copy a lot of replacements (i don't know if the perf is a real issue here, but at least it looks a bit strange to me).
Another question - how are the customers of this code supposed to use the method add ? still suppress the error (like what was happening on https://reviews.llvm.org/D24914) ?
Am i missing smth ?
cc: @klimek, @djasper.

ioeric added inline comments.Sep 28 2016, 1:44 PM
cfe/trunk/lib/Tooling/Core/Replacement.cpp
182 ↗(On Diff #72799)

First of all, this diff doesn't introduce new interfaces - they are just private helper functions, which are mostly called on conflicting replacements set with relatively small size.

Secondly, add now supports merging order-independent replacements, so it handles deduplication to some extend, i.e. identical non-insertion replacements can be deduplicated, which happens to solve the issue with clang-rename as mentioned in D24914.

cfe/trunk/lib/Tooling/Core/Replacement.cpp
182 ↗(On Diff #72799)

i see, thank you for the explanation.