ioeric added a comment. @djasper Thanks for the insight into the problem.
For the cases you listed, I have added some test cases. Case 1-3 can be handled as expected. And you are right, the behavior for case 4 is interesting. But I still agree that `addOrMerge` is confusing. As you said, the intricate behavior here is adding benign conflicting replacement in case 1-3. This is actually like `add` with assumption that benign conflicts will be resolved by combining/merging with the adjacent replacements. Maybe we can add an optional parameter to `add` interface (e.g. `mergeAdjacent`) to resolve conflicts for case 1-3 and still return error for case 4. And this might also answer @alexshap's question 2 and 3. Copying discussion from email thread: > @ioeric: > > 1. regarding B: i apologize in advance, i am not sure if it's directly > related to your patch, but i'm kinda afraid that the current approach will > shadow the issue (if it's considered to be an issue). For simplicity let's > take a look at the following example: > > Point.h: struct Point {}; > > a.cpp: include "Point.h" > > b.cpp: include "Point.h" > > clang-rename rename-all -i -old-name Point -new-name Point2 a.cpp b.cpp > Renaming failed in /Users/Alexshap/PlayRename/srcs/./include/Point.h! New > replacement: /Users/Alexshap/PlayRename/srcs/./include/Point.h: 7:+5:"Point2" > conflicts with existing replacement: > /Users/Alexshap/PlayRename/srcs/./include/Point.h: 7:+5:"Point2" > > the thing is that in the past Replacements was a typedef on std::set<...> > (if i am not mistaken) and insert() ignored duplicates. Now > Replacements.add(...) will return an error (the same replacement has already > been added). > 2. (Separate observation, I'd like to take advantage of this discussion and > ask about it) > > Replacements Replacements::merge(const Replacements &ReplacesToMerge) > > The question is the following: with the current implementation we copy all > the replacements even if the merge is "trivial" or if there are only a few > conflicts. This looks suboptimal, please, correct me if I'm wrong. > 3. Having said that - just curious - is the fallback on merge (in > addOrMergeReplacement ) really necessary in those examples ? (I mean in > change-namespace and include-fixer) > > Thanks, > Alex > … > > On Sat, Sep 10, 2016 at 9:43 PM, Daniel Jasper <djas...@google.com> wrote: > There are several things I find particularly confusing about this. > Fundamentally, Replacements refer to a specific version of the code. Say you > have a Replacements object R and a Replacement r that you want to integrate > into it. Further assume, that C0 is the initial version of the code whereas > C1 is the version after applying R. If you *add* r to R, that means that r > (specifically the offset and length in it) refer to C0. If you *merge* it, > that means that r refers to C1. Correct replacements can always be *merged*, > while *adding* might cause conflicts if they overlap with existing ones. > Thus, a function "add or merge" fundamentally doesn't make sense to me. These > are fundamentally different concepts. > > What this function (and the different implementations we already have) is > fundamentally trying to do is to have a version of *add* that resolves (some) > conflicts. It does that by assuming that r is currently referring to C0, so > it first shifts it and then calls *merge*. The fact that you can shortcut if > there isn't actually a conflict (i.e. the "addOr" part) is just an > optimization. I am fine with implementing that optimization, but it shouldn't > be part of the name. > > Now, I don't (yet) have a good name as this has very intricate behavior. And > I am not sure it is really a good idea to have this magically "resolve" all > conflicts, because I think there are different classes. I think what we want > here is to have a way to combine replacements that require a defined order, > but that don't actually conflict. Lets look at a few cases. Assume R contains > a single Replacement A and you trying to "addOrMerge" another Replacement B. > > 1. A and B are both inserts at the same code location. This seems relatively > benign and we just want a defined order > 2. A inserts at offset X and B replaces (X, X+N). Still quite benign, but we > don't want B to replace anything that A inserted, it should replace the > original text > 3. B inserts at offset X and A replaces (X, X+N). Same thing, though > interesting as we now actually have to switch the order > 4. A and B actually replace overlapping code ranges. This is really bad and I > think we should continue to report a conflict instead of doing something weird > > I'd think that your existing implementation gets #1 right but possibly only > one of #2/#3. It will also do something very interesting for #4 and I really > think what we want is to report an error. > > On Sat, Sep 10, 2016 at 5:53 PM, Eric Liu <ioe...@google.com> wrote: This > function has been implemented in several places, so I figure it might be > useful to have this function in the library. Although I'm not sure whether > this should be a class interface or just a standalone helper function. > > @alexshap: thanks for the explanation. For your point B, I'm not sure > about the example, could you elaborate a bit? E.g. which use case to be > specific? What are the headers? > > Generally, the way users of `Replacements` class adding new replacements > also matters. For example, if you have multiple insertions at the same > offset, you could concatenate them and insert as one single replacement, if > you care about performance. > > For most use cases I've seen when converting from old std::set > implementation to the current implementation, performance can be improved by > changing the way replacements are added. And the most natural way is often > the most efficient one. https://reviews.llvm.org/D24383 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits