[PATCH] D29621: Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks

[Julian Bangert via Phabricator via cfe-commits]

jbangert 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.");
----------------
sbenza wrote:
> 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.
There's a FIXME: above that addresses the need for better error handling, and this one just duplicated their workaround. 
Ultimately, clang::tooling:: should probably provide infrastructure for reporting problems (I would imagine all refactoring tools have errors occassionally, and they need some way of reporting/handling them). 


================
Comment at: lib/Tooling/RefactoringCallbacks.cpp:214
+                     << " used in replacement template not bound in Matcher \n";
+        llvm_unreachable("Unbound node in replacement template.");
+      }
----------------
sbenza wrote:
> 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.
Using llvm::report_fatal_error for now. See the above for better error handling. 


https://reviews.llvm.org/D29621