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

[Eric Liu via cfe-commits]

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.

On Sat, Sep 10, 2016, 10:54 Alexander Shaposhnikov <shal1t712 at gmail.com>
wrote:

> alexshap added a comment.
>
> 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:
>
> A. right now i see the same code (with minor changes) in several places:
>
> 1.
>
> llvm/tools/clang/tools/extra/include-fixer/IncludeFixer.cpp +382
>
>   auto R = tooling::Replacement(
>         {FilePath, Info.Range.getOffset(), Info.Range.getLength(),
>         Context.getHeaderInfos().front().QualifiedName});
>    auto Err = Replaces.add(R);
>    if (Err) {
>      llvm::consumeError(std::move(Err));
>      R = tooling::Replacement(
>          R.getFilePath(), Replaces.getShiftedCodePosition(R.getOffset()),
>          R.getLength(), R.getReplacementText());
>      Replaces = Replaces.merge(tooling::Replacements(R));
>    }
>
> 2.
>
> llvm/tools/clang/tools/extra/change-namespace/ChangeNamespace.cpp +126
> (see https://reviews.llvm.org/D24183)
>
>
>   void addOrMergeReplacement(const tooling::Replacement &R,
>                            tooling::Replacements *Replaces) {
>    auto Err = Replaces->add(R);
>    if (Err) {
>       llvm::consumeError(std::move(Err));
>       auto Replace = getReplacementInChangedCode(*Replaces, R);
>       *Replaces = Replaces->merge(tooling::Replacements(Replace));
>    }
>   }
>
> 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).
>
>
> https://reviews.llvm.org/D24383
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160910/fb553ecb/attachment.html>