Page MenuHomePhabricator

Add a batch query and replace tool based on AST matchers.
Needs ReviewPublic

Authored by jbangert on Feb 7 2017, 1:11 AM.

Details

Reviewers
klimek
Summary

This requires https://reviews.llvm.org/D29613. It adds a new tool, clang-query-replace that replaces and AST node with the result of evaluating a simple template language.

Event Timeline

jbangert created this revision.Feb 7 2017, 1:11 AM
ioeric added a subscriber: ioeric.Feb 7 2017, 1:16 AM
klimek added inline comments.Feb 7 2017, 1:38 AM
clang-query/QueryReplace.cpp
51

Shouldn't that also just return the error?

clang-query/QueryReplace.h
36–37

This doesn't seem to come up in the test? (and I don't understand what it's trying to tell me :)

ioeric added inline comments.Feb 7 2017, 2:59 AM
clang-query/QueryReplace.cpp
2

Please add license header.

38

I'd prefer using llvm::Expected<T> with an llvm::StringError.

59

return error here?

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

73

Missing // namespace ...

clang-query/replace-tool/ClangQueryReplace.cpp
77

If this parses yaml string, shouldn't it be called parseFromYAML instead?

sbenza added a subscriber: sbenza.Feb 7 2017, 7:44 AM
sbenza added inline comments.
clang-query/QueryReplace.h
36–37

In the other change the identifier was specified with ${identifier}, not %"identifier".
They should be consistent, no?

jbangert updated this revision to Diff 87580.Feb 7 2017, 6:31 PM
jbangert marked 7 inline comments as done.

Response to initial code review.

Thank you for the feedback, addressed and reformatted using llvm style.

clang-query/QueryReplace.h
36–37

I edited the comment for consistency. This indeed has an older syntax.

jbangert updated this revision to Diff 94297.Apr 5 2017, 3:33 PM

Fix command line tool.