[PATCH] D29622: Add a batch query and replace tool based on AST matchers.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: clang-query/QueryReplace.cpp:1
+#include "QueryReplace.h"
+#include "QueryParser.h"
----------------
Please add license header.


================
Comment at: clang-query/QueryReplace.cpp:37
+
+llvm::ErrorOr<QueryReplaceSpec> QueryReplaceSpec::parseFromJSON(
+    StringRef Spec) {
----------------
I'd prefer using `llvm::Expected<T>` with an `llvm::StringError`.


================
Comment at: clang-query/QueryReplace.cpp:58
+    Diag.printToStreamFull(llvm::errs());
+    llvm::errs() << "\n";
+  }
----------------
return error here?

Consider using `llvm::Error` with `llvm::StringError`.


================
Comment at: clang-query/QueryReplace.cpp:72
+}
+}
+}
----------------
Missing `// namespace ...`


================
Comment at: clang-query/replace-tool/ClangQueryReplace.cpp:76
+    llvm::ErrorOr<QueryReplaceSpec> Spec =
+        QueryReplaceSpec::parseFromJSON(SpecYaml);
+    if (error_code EC = Spec.getError()) {
----------------
If this parses yaml string, shouldn't it be called `parseFromYAML` instead?


https://reviews.llvm.org/D29622