This is an archive of the discontinued LLVM Phabricator instance.

Add ASTMatchRefactorer and ReplaceNodeWithTemplate to RefactoringCallbacks
ClosedPublic

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

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
45

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

258

Just use if since there is a continue?

263

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

304

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

Why emplace_back instead of push_back?

lib/Tooling/RefactoringCallbacks.cpp
253

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.

267

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

285

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

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

Is this covered in the test?

unittests/Tooling/RefactoringCallbacksTest.cpp
101

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

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

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

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

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

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

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

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

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