[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Samuel Benzaquen via Phabricator via cfe-commits]

sbenza added inline comments.


================
Comment at: lib/Tooling/RefactoringCallbacks.cpp:213
+        llvm::errs() << "Node " << Element.Value
+                     << " used in replacement template not bound in Matcher \n";
+        llvm_unreachable("Unbound node in replacement template.");
----------------
I don't know if stderr is the best place for this error output.
Maybe we should take a sink of some sort in the constructor.


================
Comment at: lib/Tooling/RefactoringCallbacks.cpp:214
+                     << " used in replacement template not bound in Matcher \n";
+        llvm_unreachable("Unbound node in replacement template.");
+      }
----------------
I don't think this is ok.
afaik, llvm_unreachable leads to undefined behavior if it is reached in opt mode.
This error can be triggered from user input. We should not fail that way.


================
Comment at: lib/Tooling/RefactoringCallbacks.cpp:227
+  if (NodeMap.count(FromId) == 0) {
+    llvm::errs() << "Node to be replaced " << FromId
+                 << " not bound in query.\n";
----------------
Same as above. This error can be triggered from user input.


https://reviews.llvm.org/D29621