[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.


================
Comment at: include/clang/Tooling/RefactoringCallbacks.h:61
+    MatchFinder.addMatcher(Matcher, Callback);
+    Callbacks.emplace_back(Callback);
+  }
----------------
Why emplace_back instead of push_back?


================
Comment at: lib/Tooling/RefactoringCallbacks.cpp:160
+  for (size_t Index = 0; Index < ToTemplate.size();) {
+    if (ToTemplate[Index] == '$') {
+      if (ToTemplate.substr(Index, 2) == "$$") {
----------------
We should do the parsing in the constructor, instead of on each match.
We could store a vector of {string, bool is_id;} that we just iterate during each match.
It also has the advantage of hitting the error+assert before in the run.


================
Comment at: lib/Tooling/RefactoringCallbacks.cpp:174
+            ToTemplate.substr(Index + 2, EndOfIdentifier - Index - 2);
+        if (NodeMap.count(SourceNodeName) == 0) {
+          llvm::errs()
----------------
This is making two lookups into the map when one is enough.


================
Comment at: lib/Tooling/RefactoringCallbacks.cpp:192
+      size_t NextIndex = ToTemplate.find('$', Index + 1);
+      ToText = ToText + ToTemplate.substr(Index, NextIndex - Index);
+      Index = NextIndex;
----------------
X += instead of X = X +


https://reviews.llvm.org/D29621