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

Thanks, the APIs totally make sense. And seem to fit into the other functions we have in `FixIt.h`, although the name of the file is somewhat misleading.
Mostly comments about naming and one comment about the necessity of using matchers from my side.



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


================
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
----------------
What are other tricky cases you have in mind for the future?


================
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);
----------------
Could you add an example of the API use here? The anticipated use-case are removing or textually replacing a node, right?


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


Repository:
  rC Clang

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

https://reviews.llvm.org/D58556