[PATCH] D36156: [rename] Introduce symbol occurrences

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.


================
Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:32
+/// a macro expansion.
+class SymbolOccurrence {
+public:
----------------
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)


================
Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:50
+    ///
+    /// The user will have to fixup their code manually after performing such a
+    /// rename.
----------------
Nit: s/fixup/fix/?


================
Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:65-68
+  ArrayRef<SourceLocation> getNameLocations() const { return Locations; }
+  ArrayRef<unsigned> getNameLengths() const {
+    return llvm::makeArrayRef(NameLengths, Locations.size());
+  }
----------------
Any reason not to return ranges instead here?


================
Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:72-73
+  OccurrenceKind Kind;
+  ArrayRef<SourceLocation> Locations;
+  const unsigned *NameLengths;
+};
----------------
Can we store ranges instead?


================
Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:78
+/// unit.
+class SymbolOccurrences {
+public:
----------------
Why can't we put all data in SymbolOccurrence and just use a normal container here?


Repository:
  rL LLVM

https://reviews.llvm.org/D36156