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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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"?
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.
include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h | ||
---|---|---|
33 | 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) | |
51 | Nit: s/fixup/fix/? | |
66–69 | Any reason not to return ranges instead here? | |
73–74 | Can we store ranges instead? | |
79 | Why can't we put all data in SymbolOccurrence and just use a normal container here? |
include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h | ||
---|---|---|
66–69 | Oops, this is embarrassing! I didn't realize that AtomicChange had an additional overload that accepted CharSourceRange. I'll go back to ranges. |
Sorry for the delay. The code looks good roughly, a few nits below.
include/clang/Tooling/Refactoring/Rename/SymbolName.h | ||
---|---|---|
20 | 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? | |
22 | I think this can be removed? | |
lib/Tooling/Refactoring/Rename/RenamingAction.cpp | ||
60 | 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? | |
61 | nit: For out parameter, I'd use pointer. | |
62 | 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 | ||
398 | I think just returning Visitor.getOccurrences() is sufficient -- compiler can handle it well, also using std::move would prevent copy elision. |
include/clang/Tooling/Refactoring/Rename/SymbolName.h | ||
---|---|---|
20 | 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 | ||
398 | 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. |
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 | ||
398 | 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? |
lib/Tooling/Refactoring/Rename/USRLocFinder.cpp | ||
---|---|---|
398 | Yeah, takeOccurrences would be better I think. I'll update. |
LGTM.
include/clang/Tooling/Refactoring/Rename/SymbolName.h | ||
---|---|---|
20 | Sounds good. No need to do it in this patch. |
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?