ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land.
In D62621#1529691 <https://reviews.llvm.org/D62621#1529691>, @ymandel wrote: > Thanks for the review. > > Good questions. The answer to both is that it depends on the `RangeSelector` > used. `change` itself takes exactly the range given it and modifies the > source at that range. Since these APIs are acting at the character level, I > think it could be confusing to add token-level reasoning. That said, we > could add `RangeSelector`s and/or `TextGenerator`s that are smarter about > this and ensure/add spaces around text as needed. Since all changes should > (in my opinion) be run through clang-format afterwards, there's no risk in > adding extra whitespace. For example, we could change `text()` to add > whitespace on both sides of the argument, or add a new combinator `pad` which > does that (and leave `text()` alone). So, for this example, the user would > write `insertBefore([[FOO]], pad("BAR"))` and get `BAR FOO`. > > We could similarly update `Stencil` to pad its output by default or somesuch. > > WDYT? The current design sounds reasonable to me. The macro case I mentioned is somewhat weird and I'm not even sure how often it shows up in practice. Most range selectors based on AST nodes should start and end at reasonable position that don't require any extra spaces when replaced. Let's keep the whitespace problem in mind, but not do anything about it just yet. If this actually turns out to be a problem in practice we can revisit, having some intuition what the actual tricky cases are and maybe with more approaches on how to solve them. Also LGTM, thanks! ================ Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:202 + +/// Inserts \p Replacement after \p S, automatically calculating the end +/// location of \p S. ---------------- ymandel wrote: > ilya-biryukov wrote: > > Could you elaborate on what `calculating the end location of \p S` means? > > Is the fact that the end location is calculated specific to this function, > > rather than `RangeSelector` in general? > > > > The comment is very different from `insertBefore`, suggesting the function > > behaves differently in practice. > > > > > I'm glad you bring this up, since I struggled with how best to express this. > The point is that it handles the difference between CharRange and TokenRange > and does the "right" thing. This kind of thing has bitten me and others in > the past -- it's a pretty typical newbie error to mishandle the `SourceRange` > of a node. > > That said, it is *not* doing anything more than `RangeSelector` already does, > since `RangeSelector` is already built on `CharSourceRange` rather than > `SourceRange`. So, I'd be fine dropping this mention entirely, since it's > really a property of the whole system. That is, I could replace with > "Inserts \p Replacement after \p S, leaving the source selected by \S > unchanged." > > WDYT? > > As for insertBefore -- since the beginning of a range is unambiguous, there's > no need to say anything interesting there. I like the idea of a short comment not mentioning the token range peculiarities. At the very end, that's what everyone **should** expect from the API. The fact that clang's methods require this is surprising, transformers just "boringly" do the right thing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62621/new/ https://reviews.llvm.org/D62621 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
