Page MenuHomePhabricator

[rename] Introduce symbol occurrences
ClosedPublic

Authored by arphaman on Aug 1 2017, 8:48 AM.

Details

Summary

Symbol occurrences store the results of local rename and will also be used for the global, indexed rename results. They can be converted to a set of AtomicChanges as well. This is a preparation patch for both the support of multi-piece local-renames (ObjC selectors) and the migration of clang-rename over to clang-refactor.
This is a non functional change, and I just wanted to make sure the general direction of this patch is accepted before committing.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Aug 1 2017, 8:48 AM
arphaman edited the summary of this revision. (Show Details)Aug 1 2017, 8:49 AM
kimgr added a subscriber: kimgr.Aug 1 2017, 10:28 AM

High-level comment from the peanut gallery:

The word "occurrences" is horrible to type and most people misspell it (you did great here!) Would you consider something like "SymbolMatches" or even "SymbolHits"?

High-level comment from the peanut gallery:

The word "occurrences" is horrible to type and most people misspell it (you did great here!) Would you consider something like "SymbolMatches" or even "SymbolHits"?

Sure, it would be reasonable to change it something easier, as long as it's still descriptive. "SymbolMatches" could work, although I think I'd prefer something a bit more descriptive. What about "SymbolReference"/"References"?

kimgr added a comment.Aug 2 2017, 1:01 AM

I was about to suggest SymbolLocation or even SymbolLoc, but it appears to keep track of more than locations. "Reference" sounds a little open-ended. No more ideas here, I'm afraid.

klimek added inline comments.Aug 2 2017, 3:00 AM
include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
32

I understand the exact vs. non-exact idea, but can you expand a bit on how Obj-C code looks where a rename needs to touch more than one location? (I have no idea about Obj-C unfortunately)

50

Nit: s/fixup/fix/?

65–68

Any reason not to return ranges instead here?

72–73

Can we store ranges instead?

78

Why can't we put all data in SymbolOccurrence and just use a normal container here?

arphaman added inline comments.Aug 2 2017, 4:56 AM
include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
65–68

I used ranges before, but I found that it was easier to create the atomic changes with location+lengths. Should I go back to ranges?

72–73

We could, but see above for my current reasoning.

arphaman added inline comments.Aug 2 2017, 5:18 AM
include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
65–68

Oops, this is embarrassing! I didn't realize that AtomicChange had an additional overload that accepted CharSourceRange. I'll go back to ranges.

arphaman updated this revision to Diff 109326.Aug 2 2017, 6:09 AM
arphaman marked 5 inline comments as done.

Address review comments.

hokein edited edge metadata.Aug 14 2017, 5:56 AM

Sorry for the delay. The code looks good roughly, a few nits below.

include/clang/Tooling/Refactoring/Rename/SymbolName.h
19 ↗(On Diff #109326)

An off-topic thought: currently we put everything into clang::tooling, I think we might need a separate namespace e.g. clang::tooling::refactoring in the future?

21 ↗(On Diff #109326)

I think this can be removed?

lib/Tooling/Refactoring/Rename/RenamingAction.cpp
59

Maybe add a comment for this utility function.

The name is a bit confusing, IIUC, the function actually fills the FileToReplaces, not apply the changes? maybe a better name for it?

60

nit: For out parameter, I'd use pointer.

61

nit: const auto &

lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
18

Shouldn't the definition be in clang::tooling namespace?

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
397

I think just returning Visitor.getOccurrences() is sufficient -- compiler can handle it well, also using std::move would prevent copy elision.

arphaman marked 5 inline comments as done.Aug 14 2017, 6:30 AM
arphaman added inline comments.
include/clang/Tooling/Refactoring/Rename/SymbolName.h
19 ↗(On Diff #109326)

That would be nice, I agree. Don't think it's in scope for this patch though, maybe for https://reviews.llvm.org/D36075?

lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
18

That's not necessary because of the using namespace directives above. Only standalone functions have to be defined in namespaces, out-of-line member function definitions don't have to follow that rule because they already have the class qualifier.

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
397

I'm returning by non-const reference from getOccurrences, so the compiler copies by default. I have to move either here or return by value from getOccurrences and move there. We also have warnings for redundant std::move, and they don't fire here.

arphaman updated this revision to Diff 110954.Aug 14 2017, 6:30 AM
arphaman marked an inline comment as done.

Address review comments.

hokein added inline comments.Aug 14 2017, 7:25 AM
lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
18

Ah, I see, thanks for the explanation. The constructor actually lives in "SymbolOccurrence". T

I'd like to use a more familiar way: namespace clang { namespace tooling { ... } }

lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
397

Ok, didn't notice that getOccurance() returns non-const reference.

I think the method getOccurances may have potential effect -- the clients could change the Occurences member variable of USRLocFindingASTVisitor, after getting the non-const reference.

Another option is to rename getOccurances to takeOccurrences and return by value:

SymbolOccurrences takeOccurrences() { return std::move(Occurrences); }

What do you think?

arphaman marked an inline comment as done.Aug 14 2017, 8:08 AM
arphaman added inline comments.
lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
397

Yeah, takeOccurrences would be better I think. I'll update.

arphaman updated this revision to Diff 110964.Aug 14 2017, 8:09 AM
arphaman marked an inline comment as done.

Use takeOccurrences

hokein accepted this revision.Aug 14 2017, 8:36 AM

LGTM.

include/clang/Tooling/Refactoring/Rename/SymbolName.h
19 ↗(On Diff #109326)

Sounds good. No need to do it in this patch.

This revision is now accepted and ready to land.Aug 14 2017, 8:36 AM
This revision was automatically updated to reflect the committed changes.