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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to