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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

ymandel marked 4 inline comments as done.
ymandel added inline comments.


================
Comment at: clang/include/clang/Tooling/FixIt.h:60
+// future to include more associated text (like comments).
+CharSourceRange getSourceRangeAuto(const Stmt &S, ASTContext &Context);
+
----------------
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?


================
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
----------------
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.


================
Comment at: clang/include/clang/Tooling/FixIt.h:77
+template <typename T>
+StringRef getTextAuto(const T &Node, ASTContext &Context) {
+  return internal::getText(getSourceRangeAuto(Node, Context), Context);
----------------
ilya-biryukov wrote:
> Could you add an example of the API use here? The anticipated use-case are removing or textually replacing a node, right?
Yes, that's right.  Will do (on next edit, once we've resolved naming, etc.)


================
Comment at: clang/lib/Tooling/FixIt.cpp:52
+
+  auto NotCondition = unless(hasCondition(equalsNode(&S)));
+  auto Standalone =
----------------
ilya-biryukov wrote:
> Do you expect this function to be on the hot path?
> If so, I'd advice against using the matchers here. They do add enough overhead to be avoided in hot functions.
> 
> I guess the problem is that we can't get a hold of the parent node with using the matchers, right?
> Not sure if there's an easy way out of it in that case.
In the context of transformer, I expect that this will be called in proportion to the number of times that a match callback is invoked.  so, I expect there to already be matcher use in the control flow.

Yes, I'm using the matchers almost entirely for the hasParent() functionality.

Note that we can change the order of the guards in lines 67-69 and first check for a trailing semi which I'd guess is cheaper than calling the matcher. In that case, matching will only happen for expressions followed by semicolons.

Alternatively, I think I could restructure the uses of this function to *provide* the parent node. In that case, callers can decide what makes the most sense performance-wise for getting the parent. 


Repository:
  rC Clang

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

https://reviews.llvm.org/D58556