arphaman marked 5 inline comments as done.
arphaman added inline comments.

================
Comment at: include/clang/Tooling/Refactoring/Rename/SymbolName.h:19
+namespace clang {
+namespace tooling {
+
----------------
hokein wrote:
> 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? 
That would be nice, I agree. Don't think it's in scope for this patch though, 
maybe for https://reviews.llvm.org/D36075?


================
Comment at: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp:17
+
+SymbolOccurrence::SymbolOccurrence(const SymbolName &Name, OccurrenceKind Kind,
+                                   ArrayRef<SourceLocation> Locations)
----------------
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.


================
Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:398
   Visitor.TraverseDecl(Decl);
-  return Visitor.getLocationsFound();
+  return std::move(Visitor.getOccurrences());
 }
----------------
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.


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

Reply via email to