Page MenuHomePhabricator

Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

jbangert created this revision.Feb 7 2017, 1:09 AM
ioeric added a subscriber: ioeric.Feb 7 2017, 1:16 AM
ioeric added inline comments.Feb 7 2017, 2:51 AM
lib/Tooling/RefactoringCallbacks.cpp
42 ↗(On Diff #87372)

Could you add a comment here explaining why the replacements in callbacks need to be cleared?

165 ↗(On Diff #87372)

Just use if since there is a continue?

170 ↗(On Diff #87372)

You might want to use llvm_unreachable instead so that it also bails out in non-assertion build.

211 ↗(On Diff #87372)

Looks like you are using clang-format with Google-style. Please reformat your changes with LLVM style.

sbenza added a subscriber: sbenza.Feb 7 2017, 7:40 AM
sbenza added inline comments.
include/clang/Tooling/RefactoringCallbacks.h
61 ↗(On Diff #87372)

Why emplace_back instead of push_back?

lib/Tooling/RefactoringCallbacks.cpp
160 ↗(On Diff #87372)

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.

174 ↗(On Diff #87372)

This is making two lookups into the map when one is enough.

192 ↗(On Diff #87372)

X += instead of X = X +

jbangert updated this revision to Diff 87574.Feb 7 2017, 6:15 PM

Address code feedback & Reformat with clang-format --style llvm.

jbangert updated this revision to Diff 87575.Feb 7 2017, 6:16 PM
  • change to push_back
jbangert marked 5 inline comments as done.Feb 7 2017, 6:29 PM

Thank you for the initial feedback!

include/clang/Tooling/RefactoringCallbacks.h
61 ↗(On Diff #87372)

Changed to push_back. Is there ever an advantage to using push_back over emplace_back (the latter falls back to a copy constructor).

jbangert marked 2 inline comments as done.Feb 7 2017, 6:29 PM

(marking other comments as done).

jbangert updated this revision to Diff 87579.Feb 7 2017, 6:30 PM
  • use iter
ioeric added inline comments.Feb 8 2017, 7:48 AM
lib/Tooling/RefactoringCallbacks.cpp
160 ↗(On Diff #87579)

Is this covered in the test?

unittests/Tooling/RefactoringCallbacksTest.cpp
101 ↗(On Diff #87579)

Have you rerun the tests? Does this still build?

jbangert updated this revision to Diff 87879.Feb 9 2017, 2:12 PM
  • fix test
jbangert marked 2 inline comments as done.Feb 9 2017, 2:13 PM

Fixed the test -- I am still getting used to the different ninja test targets.

lib/Tooling/RefactoringCallbacks.cpp
160 ↗(On Diff #87579)

Now it is.

ioeric accepted this revision.Feb 10 2017, 1:32 AM

Looks good. Could you also add test cases where ReplaceNodeWithTemplate::create fails to parse templates?

This revision is now accepted and ready to land.Feb 10 2017, 1:32 AM
sbenza added inline comments.Feb 10 2017, 1:20 PM
include/clang/Tooling/RefactoringCallbacks.h
61 ↗(On Diff #87372)

push_back and emplace_back are equivalent when the value you are passing is already a T.
In that case, push_back should be used from the principle of using the least powerful tool that can do the job you want.
emplace_back allows for calls to explicit constructors, which makes it more powerful in ways that might be surprising to the reader.
Eg:

std::vector<std::vector<int>> v;
... many lines below ...
v.push_back(1);  // fails
v.emplace_back(1);  // works, but it didn't add a 1 to the vector.
jbangert updated this revision to Diff 89420.Feb 22 2017, 2:23 PM
jbangert marked an inline comment as done.
  • additional tests
jbangert accepted this revision.Feb 22 2017, 2:25 PM

Thanks, added tests for parser failures.

sbenza added inline comments.Feb 22 2017, 2:48 PM
lib/Tooling/RefactoringCallbacks.cpp
213 ↗(On Diff #87879)

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.

214 ↗(On Diff #87879)

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.

227 ↗(On Diff #87879)

Same as above. This error can be triggered from user input.

jbangert updated this revision to Diff 89730.Feb 24 2017, 3:05 PM
jbangert marked an inline comment as done.

use llvm::report_fatal_error instead of unreachable.

jbangert added inline comments.Feb 24 2017, 3:15 PM
lib/Tooling/RefactoringCallbacks.cpp
213 ↗(On Diff #87879)

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).

214 ↗(On Diff #87879)

Using llvm::report_fatal_error for now. See the above for better error handling.

ioeric edited reviewers, added: sbenza; removed: jbangert.Feb 25 2017, 10:59 AM
jbangert updated this revision to Diff 94282.Apr 5 2017, 2:09 PM
jbangert accepted this revision.Apr 5 2017, 2:11 PM
jbangert marked 2 inline comments as done.
jbangert added inline comments.
unittests/Tooling/RefactoringCallbacksTest.cpp
101 ↗(On Diff #87579)

ninja check-clang-tools works.

ioeric added a comment.Apr 6 2017, 2:59 AM

Still LGTM. Please run ninja check-clang and fix the warning. I'll land this patch for you afterwards.

lib/Tooling/RefactoringCallbacks.cpp
222 ↗(On Diff #94282)
ninja check-clang
RefactoringCallbacks.cpp:222:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

Please remove the default.

jbangert updated this revision to Diff 98203.May 8 2017, 2:00 PM

Ran check-clang

This revision was automatically updated to reflect the committed changes.