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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
