djasper 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, ---------------- ioeric wrote: > 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? I think that might not be necessary. If so, we might need to see a few use cases to be sure to get it right. https://reviews.llvm.org/D27054 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
