<div dir="ltr"><div>@ioeric:<br></div>1.<div><div>regarding B:</div><div>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</div><div>the issue (if it's considered to be an issue).</div><div>For simplicity let's take a look at the following example:</div><div><br></div><div>Point.h:</div><div> struct Point {};</div><div><br></div><div>a.cpp:</div><div> include "Point.h"</div><div><br></div><div>b.cpp:</div><div> include "Point.h"</div><div><br></div><div><div>clang-rename rename-all -i -old-name Point -new-name Point2 a.cpp b.cpp</div><div>Renaming failed in /Users/Alexshap/PlayRename/srcs/./include/Point.h! New replacement:</div><div>/Users/Alexshap/PlayRename/srcs/./include/Point.h: 7:+5:"Point2"</div><div>conflicts with existing replacement:</div><div>/Users/Alexshap/PlayRename/srcs/./include/Point.h: 7:+5:"Point2"</div></div><div><br></div><div>the thing is that in the past Replacements was a typedef on std::set<...> (if i am not mistaken)</div><div>and insert() ignored duplicates. Now Replacements.add(...) will return an error (the same replacement has already been added).</div><div><br></div><div>2. </div><div>(Separate observation, I'd like to take advantage of this discussion and ask about it)</div><div><br></div><div>Replacements<span style="color:rgb(0,0,0);font-family:monospace,fixed;font-size:13px;line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)"> </span>Replacements::merge<span style="color:rgb(0,0,0);font-family:monospace,fixed;font-size:13px;line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)">(</span><span style="font-family:monospace,fixed;font-size:13px;line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)"><font color="#008000">const</font></span><span style="color:rgb(0,0,0);font-family:monospace,fixed;font-size:13px;line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)"> </span>Replacements<span style="color:rgb(0,0,0);font-family:monospace,fixed;font-size:13px;line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)"> &ReplacesToMerge)</span><br></div><div><span style="color:rgb(0,0,0);font-family:monospace,fixed;font-size:13px;line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)"><br></span></div><div><font color="#000000" face="monospace, fixed"><span style="line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)">The question is the following: with the current implementation we copy</span></font></div><div><font color="#000000" face="monospace, fixed"><span style="line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)">all the replacements even if the merge is "trivial" or if there are only a few conflicts.</span></font></div><div><font color="#000000" face="monospace, fixed"><span style="line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)">This looks suboptimal, please, correct me if I'm wrong.</span></font></div><div><br></div><div><font color="#000000" face="monospace, fixed"><span style="line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)">3. Having said that - just curious</span></font></div><div><font color="#000000" face="monospace, fixed"><span style="line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)">- is the fallback on merge (in </span></font><span style="color:rgb(0,0,0);font-family:menlo,consolas,monaco,monospace;font-size:10px;background-color:rgb(253,243,218)">addOrMergeReplacement </span><span style="line-height:13px;white-space:pre-wrap;color:rgb(0,0,0);font-family:monospace,fixed;background-color:rgb(251,252,253)">) really necessary in those examples ?</span></div><div><font color="#000000"><span style="font-family:monospace,fixed;line-height:13px;white-space:pre-wrap;background-color:rgb(251,252,253)">(I mean in change-namespace and </span><span style="white-space:pre-wrap;line-height:18.85px;background-color:rgb(251,252,253)"><font face="segoe ui, segoe ui web regular, segoe ui symbol, helvetica neue, helvetica, arial, sans-serif">include-fixer)</font></span></font></div><div><br></div><div><br></div><div>Thanks,</div><div>Alex</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Sep 10, 2016 at 9:43 PM, Daniel Jasper <span dir="ltr"><<a href="mailto:djasper@google.com" target="_blank">djasper@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div>1. A and B are both inserts at the same code location. This seems relatively benign and we just want a defined order</div><div>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</div><div>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</div><div>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</div><div><br></div><div>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.</div></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Sep 10, 2016 at 5:53 PM, Eric Liu <span dir="ltr"><<a href="mailto:ioeric@google.com" target="_blank">ioeric@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. <div><br></div><div>@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?</div><div><br></div><div>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. </div><div><br></div><div>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.<div><div><br><br><div class="gmail_quote"><div dir="ltr">On Sat, Sep 10, 2016, 10:54 Alexander Shaposhnikov <<a href="mailto:shal1t712@gmail.com" target="_blank">shal1t712@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">alexshap added a comment.<br>
<br>
disclaimer: i don't know if this method is the right thing to add (to be honest i would prefer a better interface but don't have any good suggestions on my mind at the moment), i see several issues (IMO, i might be mistaken, apologize in advance) with the current interface of Replacements. I will not list all of them, but want to add some context:<br>
<br>
A. right now i see the same code (with minor changes) in several places:<br>
<br>
1.<br>
<br>
llvm/tools/clang/tools/extra/i<wbr>nclude-fixer/IncludeFixer.cpp +382<br>
<br>
auto R = tooling::Replacement(<br>
{FilePath, Info.Range.getOffset(), Info.Range.getLength(),<br>
Context.getHeaderInfos().front<wbr>().QualifiedName});<br>
auto Err = Replaces.add(R);<br>
if (Err) {<br>
llvm::consumeError(std::move(<wbr>Err));<br>
R = tooling::Replacement(<br>
R.getFilePath(), Replaces.getShiftedCodePositio<wbr>n(R.getOffset()),<br>
R.getLength(), R.getReplacementText());<br>
Replaces = Replaces.merge(tooling::Replac<wbr>ements(R));<br>
}<br>
<br>
2.<br>
<br>
llvm/tools/clang/tools/extra/c<wbr>hange-namespace/ChangeNamespac<wbr>e.cpp +126 (see <a href="https://reviews.llvm.org/D24183" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2418<wbr>3</a>)<br>
<br>
<br>
void addOrMergeReplacement(const tooling::Replacement &R,<br>
tooling::Replacements *Replaces) {<br>
auto Err = Replaces->add(R);<br>
if (Err) {<br>
llvm::consumeError(std::move(E<wbr>rr));<br>
auto Replace = getReplacementInChangedCode(*R<wbr>eplaces, R);<br>
*Replaces = Replaces->merge(tooling::Repla<wbr>cements(Replace));<br>
}<br>
}<br>
<br>
B. For replacements in headers we can get into if(Err) branch quite often because the same replacement can be generated multiple times (if that header is included into several *.cpp files).<br>
<br>
<br>
<a href="https://reviews.llvm.org/D24383" rel="noreferrer" target="_blank">https://reviews.llvm.org/D2438<wbr>3</a><br>
<br>
<br>
<br>
</blockquote></div></div></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>