Page MenuHomePhabricator

[refactor] Move clang-rename to Clang
ClosedPublic

Authored by arphaman on Jun 27 2017, 9:17 AM.

Details

Summary

The core engine of clang-rename will be used for local and global renames in the new refactoring engine (http://lists.llvm.org/pipermail/cfe-dev/2017-June/054286.html).

I haven't renamed the files/cleaned up the code in this patch, do you think I should? E.g. SymbolOccurrenceFinder instead of USRLocFinder.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jun 27 2017, 9:17 AM
klimek edited edge metadata.Jun 28 2017, 5:13 AM

The main thing I'm concerned about is having the main code in core, but having all tests in tools-extra. I think if we go that route we should also move clang-rename and its tests to core. Thoughts?

ioeric edited edge metadata.Jun 28 2017, 6:39 AM

The main thing I'm concerned about is having the main code in core, but having all tests in tools-extra. I think if we go that route we should also move clang-rename and its tests to core. Thoughts?

Agreed. And if we are also moving rename-related libraries, it might also make sense to put them into a sub-directory like lib/Tooling/Refactoring/Rename.

The main thing I'm concerned about is having the main code in core, but having all tests in tools-extra. I think if we go that route we should also move clang-rename and its tests to core. Thoughts?

Would it be better if I make a patch for the clang-refactor tool first and then update this patch to support some of clang-rename's functionality in clang-refactor and move the tests over? Or should I just move all of clang-rename and its tests to clang's repository first and then focus on clang-refactor?

The main thing I'm concerned about is having the main code in core, but having all tests in tools-extra. I think if we go that route we should also move clang-rename and its tests to core. Thoughts?

Would it be better if I make a patch for the clang-refactor tool first and then update this patch to support some of clang-rename's functionality in clang-refactor and move the tests over? Or should I just move all of clang-rename and its tests to clang's repository first and then focus on clang-refactor?

Given the end goal is clear, I'm fine with either :)

alexshap added inline comments.Jun 29 2017, 9:20 PM
tools/extra/clang-rename/tool/ClangRename.cpp
167 ↗(On Diff #104189)
  1. nit: this line caught my eye (not directly related to the main purpose of this diff):

getUSRSpellings returns ArrayRef, and ArrayRef has (see the line 271 in include/llvm/ADT/ArrayRef.h) a conversion operator to std::vector<T> (by value). So while it's correct this copy seems to be unnecessary (pls, correct me if i am missing smth) and the fix is easy. If you want i can send a separate diff or i don't mind smb else fixing it.

  1. nit: USRFindingAction public methods (getUSRSpellings etc) except newASTConsumer should be const

I think I'd rather move clang-rename first.

tools/extra/clang-rename/tool/ClangRename.cpp
167 ↗(On Diff #104189)

Thanks for the feedback! I agree with your comments, but would prefer to leave them for a separate patch. I have some similar fixes for some of this code as well.

arphaman updated this revision to Diff 104840.Jun 30 2017, 4:01 AM
arphaman retitled this revision from [refactor] Move the core of clang-rename to lib/Tooling/Refactoring to [refactor] Move clang-rename to Clang.
  • Move all of clang-rename to Clang's repository.
  • Use the Rename subdirectory in the refactoring library.
This revision is now accepted and ready to land.Jun 30 2017, 7:22 AM
This revision was automatically updated to reflect the committed changes.

I didn't notice that ClassReplacements.cpp had a dependency on clang-apply-replacements. I moved it back to clang-tools-extra to the clang-apply-replacements tests.