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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
