hokein added inline comments.
================
Comment at: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp:17
+
+SymbolOccurrence::SymbolOccurrence(const SymbolName &Name, OccurrenceKind Kind,
+ ArrayRef<SourceLocation> Locations)
----------------
arphaman wrote:
> hokein wrote:
> > Shouldn't the definition be in clang::tooling namespace?
> 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.
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 { ...
} }`
================
Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:398
Visitor.TraverseDecl(Decl);
- return Visitor.getLocationsFound();
+ return std::move(Visitor.getOccurrences());
}
----------------
arphaman wrote:
> hokein wrote:
> > I think just returning `Visitor.getOccurrences()` is sufficient -- compiler
> > can handle it well, also using std::move would prevent copy elision.
> 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.
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?
Repository:
rL LLVM
https://reviews.llvm.org/D36156
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
