ymandel marked an inline comment as done. ymandel added a comment. Thanks for the review.
In D62621#1528714 <https://reviews.llvm.org/D62621#1528714>, @ilya-biryukov wrote: > Are there any corner cases with handling whitespace that we might want to > handle or document them if we don't handle them? > E.g. if the range ends up being a name of an expanded macro , it's probably > very easy to glue the macro name with whatever you're inserting. > > #define FOO 1+2; > #define BAR 2+3; > > [[FOO]] // [[ ]] marks the selected range > > > would `insertBefore([[FOO]], "BAR")` end up being `BARFOO` or `BAR FOO`? In D62621#1528716 <https://reviews.llvm.org/D62621#1528716>, @ilya-biryukov wrote: > I guess the question also applies to `change`, although it did not occur to > me before. 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? ================ Comment at: clang/include/clang/Tooling/Refactoring/Transformer.h:202 + +/// Inserts \p Replacement after \p S, automatically calculating the end +/// location of \p S. ---------------- 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. 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
