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

[Yitzhak Mandelbaum via Phabricator via cfe-commits]

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


================
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
----------------
kimgr wrote:
> ymandel wrote:
> > kimgr wrote:
> > > 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.
> > > Peanut gallery comment on this:
> > > 
> > > > 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++?
> > > 
> > > Would it make sense to let callers choose what level of expansion they want with a flag mask? Somehow I think that makes it easier to name the function, too, e.g.:
> > > ```
> > > StringRef getExpandedRange(const Stmt &S, ASTContext &Context, ExpansionFlags Flags);
> > > ```
> > > 
> > Yes, I think that's a good idea. I even like the name except that it will likely be confused with Expansion locs.  Maybe `getExtendedRange`?
> Extended sounds good to me too! 
I went with "getExtended..." but ended up drastically simplifying the function, since the "smarts" turned out to be not smart enough. Fundamentally, the caller needs to know when to look for a trailing token (and which one).


================
Comment at: clang/lib/Tooling/FixIt.cpp:52
+
+  auto NotCondition = unless(hasCondition(equalsNode(&S)));
+  auto Standalone =
----------------
ymandel wrote:
> 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. 
Removed, so no longer relevant.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58556