[PATCH] D58556: [LibTooling] Add "smart" retrieval of AST-node source to FixIt library

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Ah, had some comments and forgot to send them out before I went on vacation :-(



================
Comment at: clang/include/clang/Tooling/FixIt.h:60
+// future to include more associated text (like comments).
+CharSourceRange getSourceRangeAuto(const Stmt &S, ASTContext &Context);
+
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > Do you have alternative names in mind? It would be nice to (1) not mention the SourceRange now that we return CharSourceRange, (2) change "auto" to something more descriptive.
> > 
> > Was thinking about `getNodeRange()` or `getSpannedRange()`, but that completely misses the "auto" part (maybe it's fine, though).
> > WDYT? Maybe other ideas?
> I completely agree. I went through quite a few iterations on this name and disliked this one the least.  ;)  I think you're right, though, that once we're choosing a different name, the "auto" part doesn't really need to be in it.  I like `getSpannedRange` better than this. Other thoughts:
> 
> getLogicalRange
> getExtendedRange
> getAssociatedRange
> 
> any preference?
`getSpannedRange` and `getAssociatedRange` would be my favorites.
Both seem to leave no room for interpretation.


================
Comment at: clang/include/clang/Tooling/FixIt.h:73
+// context. In contrast with \p getText(), this function selects a source range
+// "automatically", extracting text that a reader might intuitively associate
+// with a node.  Currently, only specialized for \p clang::Stmt, where it will
----------------
ymandel wrote:
> ilya-biryukov wrote:
> > What are other tricky cases you have in mind for the future?
> I just assumed that we'd hit more as we dig into them, but, I'm not so sure now.  The two others I can think of offhand are 1) extending to include associated comments, 2) semicolons after declarations.  Commas present a similar challenge (if a bit simpler) when used in a list (vs. the comma operator).  Are there any other separators in C++? 
> 
> At a higher level, it would be nice to align this with your work on tree transformations. That is, think of these functions as short-term shims to simulate the behavior we'll ultimately get from that new library. However, it might be premature to consider those details here.
Would it be fair to characterize those as "the text range that you needs to be removed when removing a node"?
Trying to come up with a comment that does not rely on the intuition, since that might differ from one reader to the other.

Maybe move this comment to `getSourceRangeAuto` and in this function's comment simply mention that we return the text for the node covered with `getSourceRangeAuto`?

> At a higher level, it would be nice to align this with your work on tree transformations. That is, think of these functions as short-term shims to simulate the behavior we'll ultimately get from that new library. However, it might be premature to consider those details here.
Agree, we can figure it out as soon as the syntax trees are in a good enough shape.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58556/new/

https://reviews.llvm.org/D58556