[PATCH] D24383: Add addOrMerge interface to tooling::Replacements.

[Eric Liu via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D24383#539942, @djasper wrote:

> Ok.. Thought some more and discussed with Manuel.
>
> I think we should do a partial solution for now. I still think addOrMerge is harmful and it is always never the right thing to use. The codepaths that currently implement some version of it, should likely reconsider.
>
> What I think we should implement is that some combinations of inserts and replacements are accepted by "add". Specifically, imagine you have an original text "aa" and a replacement that replaces it to "bb". Now, imagine you have an additional insert of "c" at offsets 0, 1 or 2:
>
> - Offset 0: Not ambiguous, result should be "cbb".
> - Offset 1: Conflict, report error.
> - Offset 2: Not ambiguous, result should be "bbc"
>
>   So basically, inserts adjacent to replacements (including deletions) have a well defined order and should be supported.
>
>   Two replacements still need a defined order and we should probably error for now. I have some ideas on how to solve this. The most intuitive one might be to have a pair functions "StringRef getReplacement(unsigned offset)"/"void setReplacement(unsigned offset, StringRef text)", that control the replacement at a given offset. A replacement object shouldn't have more than one insert at any given offset. But again, I think that's a problem we can get to later. I think most tools that currently would insert stuff at the same location are actually wrong and we should continue to error.


If I understand correctly, `add` should support:

- adding non-conflict replacement as before.
- adding insertion that is adjacent to but not contained in an existing replacement with length>0 (deletion, actual replacement), and vice versa. The replacement should always change the original code instead of inserted text.

But why don't we want to support inserting twice at the same offset? It seems reasonable to me to assume that insertions can be ordered by the time they are added. And there are also reasonable use cases to insert at the same offset. For example, a fix or tool that generates parentheses for expressions can easily generate two '(' at the same offset, e.g. `A && B || C && D`  -> `((A && B) || (C && D))`. Of course, this makes it impossible to order the later insertion before existing insertions with `add`, but I don't think we want to support that since it doesn't seem to be a reasonable way to add replacements. I've seen tools that relied on the previous `set` implementation to achieve this (i.e. insert before existing insertions), but the code is very weird and can be easily refactored to work without this feature. But if we must support it, we might want to an `insertBefore` option (default=false) to `add` interface that makes add new insertion before the existing/conflicting insertion.

> Similarly, we never want to merge overlapping replacements, but we should ensure that directly adjacent ones work, e.g. "aa" can be convert into "bc" with two replacements (one for the first character, one for the second) that are combined with add().


I think this is already true with the current implementation of `add`.


https://reviews.llvm.org/D24383