ioeric added inline comments.
================ Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94 + /// \brief Adds a replacement that inserts \p Text at \p Loc. If this + /// insertion conflicts with an existing insertion (at the same position), + /// this will be inserted before the existing insertion. If the conflicting + /// replacement is not an insertion, an error is returned. + /// + /// \returns An llvm::Error carrying ReplacementError on error. + llvm::Error insertBefore(const SourceManager &SM, SourceLocation Loc, ---------------- djasper wrote: > ioeric wrote: > > djasper wrote: > > > Do we currently actually need these functions? They seem a bit dangerous. > > I think these functions are helpful. Insertion conflict is by far the most > > common conflict I've seen, and several users have asked for ways to resolve > > this. And generally, resolving such conflict is not straight-forward. Also > > notice that these interfaces only resolve insertion conflict, other > > conflicts (e.g. overlaps) will still raise errors. > > > > Although inserting all texts at one location as a single replacement is > > preferred, I've seen several tools that use these in some way, i.e. > > generating all insertions at the same location is hard or impossible. > I still have several concerns: > 1. This interface actually exposes two different layers: It has special > function to insert text and headers and also the functions that add or merge > replacements. IMO, that's not ideal. > 2. There is now no function insert() if users actually want to get the error > for a conflict. Yes, they can then instead use addReplacement, but again, > that's a completely separate layer of the interface. > 3. The logic of before after AND that this only applies to two conflicting > insertions is a bit hard to explain. > > Not sure what the right trade-off for all of these is. Maybe we should just > remove addReplacement and mergeReplacements and instead add a replace() > function? Maybe we should just have a single insert() function with an > optional fourth parameter that can be used to control conflict resolution. `replace` + `insert` sounds like a good way to go. Do we want to support resolving general conflicts besides insertions though? https://reviews.llvm.org/D27054 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
