[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Alex Lorenz via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL310853: [rename] Introduce symbol occurrences (authored by 
arphaman).

Changed prior to commit:
  https://reviews.llvm.org/D36156?vs=110964=110999#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D36156

Files:
  cfe/trunk/include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  cfe/trunk/include/clang/Tooling/Refactoring/Rename/SymbolName.h
  cfe/trunk/include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
  cfe/trunk/include/clang/Tooling/Refactoring/Rename/USRLocFinder.h
  cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
  cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  cfe/trunk/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
  cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

Index: cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
===
--- cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
+++ cfe/trunk/lib/Tooling/Refactoring/CMakeLists.txt
@@ -3,6 +3,7 @@
 add_clang_library(clangToolingRefactor
   AtomicChange.cpp
   Rename/RenamingAction.cpp
+  Rename/SymbolOccurrences.cpp
   Rename/USRFinder.cpp
   Rename/USRFindingAction.cpp
   Rename/USRLocFinder.cpp
Index: cfe/trunk/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -0,0 +1,37 @@
+//===--- SymbolOccurrences.cpp - Clang refactoring library ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang;
+using namespace tooling;
+
+SymbolOccurrence::SymbolOccurrence(const SymbolName , OccurrenceKind Kind,
+   ArrayRef Locations)
+: Kind(Kind) {
+  ArrayRef NamePieces = Name.getNamePieces();
+  assert(Locations.size() == NamePieces.size() &&
+ "mismatching number of locations and lengths");
+  assert(!Locations.empty() && "no locations");
+  if (Locations.size() == 1) {
+RangeOrNumRanges = SourceRange(
+Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
+return;
+  }
+  MultipleRanges = llvm::make_unique(Locations.size());
+  RangeOrNumRanges.setBegin(
+  SourceLocation::getFromRawEncoding(Locations.size()));
+  for (const auto  : llvm::enumerate(Locations)) {
+MultipleRanges[Loc.index()] = SourceRange(
+Loc.value(),
+Loc.value().getLocWithOffset(NamePieces[Loc.index()].size()));
+  }
+}
Index: cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ cfe/trunk/lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -23,6 +23,7 @@
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Core/Lookup.h"
 #include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "clang/Tooling/Refactoring/Rename/USRFinder.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -68,11 +69,9 @@
 
   // Non-visitors:
 
-  // \brief Returns a list of unique locations. Duplicate or overlapping
-  // locations are erroneous and should be reported!
-  const std::vector () const {
-return LocationsFound;
-  }
+  /// \brief Returns a set of unique symbol occurrences. Duplicate or
+  /// overlapping occurrences are erroneous and should be reported!
+  SymbolOccurrences takeOccurrences() { return std::move(Occurrences); }
 
 private:
   void checkAndAddLocation(SourceLocation Loc) {
@@ -82,17 +81,18 @@
 StringRef TokenName =
 Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
  Context.getSourceManager(), Context.getLangOpts());
-size_t Offset = TokenName.find(PrevName);
+size_t Offset = TokenName.find(PrevName.getNamePieces()[0]);
 
 // The token of the source location we find actually has the old
 // name.
 if (Offset != StringRef::npos)
-  LocationsFound.push_back(BeginLoc.getLocWithOffset(Offset));
+  Occurrences.emplace_back(PrevName, SymbolOccurrence::MatchingSymbol,
+   BeginLoc.getLocWithOffset(Offset));
   }
 
   const std::set USRSet;
-  const std::string PrevName;
-  std::vector LocationsFound;
+  const SymbolName PrevName;
+  SymbolOccurrences Occurrences;
   const ASTContext 
 };
 
@@ -391,12 +391,11 @@
 
 } // namespace

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Haojian Wu via Phabricator via cfe-commits 
hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

LGTM.




Comment at: include/clang/Tooling/Refactoring/Rename/SymbolName.h:19
+namespace clang {
+namespace tooling {
+

arphaman wrote:
> 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?
Sounds good. No need to do it in this patch.


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman updated this revision to Diff 110964.
arphaman marked an inline comment as done.
arphaman added a comment.

Use `takeOccurrences`


Repository:
  rL LLVM

https://reviews.llvm.org/D36156

Files:
  include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  include/clang/Tooling/Refactoring/Rename/SymbolName.h
  include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
  include/clang/Tooling/Refactoring/Rename/USRLocFinder.h
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -23,6 +23,7 @@
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Core/Lookup.h"
 #include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "clang/Tooling/Refactoring/Rename/USRFinder.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -68,11 +69,9 @@
 
   // Non-visitors:
 
-  // \brief Returns a list of unique locations. Duplicate or overlapping
-  // locations are erroneous and should be reported!
-  const std::vector () const {
-return LocationsFound;
-  }
+  /// \brief Returns a set of unique symbol occurrences. Duplicate or
+  /// overlapping occurrences are erroneous and should be reported!
+  SymbolOccurrences takeOccurrences() { return std::move(Occurrences); }
 
 private:
   void checkAndAddLocation(SourceLocation Loc) {
@@ -82,17 +81,18 @@
 StringRef TokenName =
 Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
  Context.getSourceManager(), Context.getLangOpts());
-size_t Offset = TokenName.find(PrevName);
+size_t Offset = TokenName.find(PrevName.getNamePieces()[0]);
 
 // The token of the source location we find actually has the old
 // name.
 if (Offset != StringRef::npos)
-  LocationsFound.push_back(BeginLoc.getLocWithOffset(Offset));
+  Occurrences.emplace_back(PrevName, SymbolOccurrence::MatchingSymbol,
+   BeginLoc.getLocWithOffset(Offset));
   }
 
   const std::set USRSet;
-  const std::string PrevName;
-  std::vector LocationsFound;
+  const SymbolName PrevName;
+  SymbolOccurrences Occurrences;
   const ASTContext 
 };
 
@@ -391,12 +391,11 @@
 
 } // namespace
 
-std::vector
-getLocationsOfUSRs(const std::vector , StringRef PrevName,
-   Decl *Decl) {
+SymbolOccurrences getOccurrencesOfUSRs(ArrayRef USRs,
+   StringRef PrevName, Decl *Decl) {
   USRLocFindingASTVisitor Visitor(USRs, PrevName, Decl->getASTContext());
   Visitor.TraverseDecl(Decl);
-  return Visitor.getLocationsFound();
+  return Visitor.takeOccurrences();
 }
 
 std::vector
Index: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- /dev/null
+++ lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -0,0 +1,37 @@
+//===--- SymbolOccurrences.cpp - Clang refactoring library ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang;
+using namespace tooling;
+
+SymbolOccurrence::SymbolOccurrence(const SymbolName , OccurrenceKind Kind,
+   ArrayRef Locations)
+: Kind(Kind) {
+  ArrayRef NamePieces = Name.getNamePieces();
+  assert(Locations.size() == NamePieces.size() &&
+ "mismatching number of locations and lengths");
+  assert(!Locations.empty() && "no locations");
+  if (Locations.size() == 1) {
+RangeOrNumRanges = SourceRange(
+Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
+return;
+  }
+  MultipleRanges = llvm::make_unique(Locations.size());
+  RangeOrNumRanges.setBegin(
+  SourceLocation::getFromRawEncoding(Locations.size()));
+  for (const auto  : llvm::enumerate(Locations)) {
+MultipleRanges[Loc.index()] = SourceRange(
+Loc.value(),
+Loc.value().getLocWithOffset(NamePieces[Loc.index()].size()));
+  }
+}
Index: lib/Tooling/Refactoring/Rename/RenamingAction.cpp
===
--- lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -24,14 +24,54 @@
 #include "clang/Tooling/Refactoring.h"

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman marked an inline comment as done.
arphaman added inline comments.



Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:398
   Visitor.TraverseDecl(Decl);
-  return Visitor.getLocationsFound();
+  return std::move(Visitor.getOccurrences());
 }

hokein wrote:
> 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?
Yeah, `takeOccurrences` would be better I think. I'll update.


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Haojian Wu via Phabricator via cfe-commits 
hokein added inline comments.



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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman updated this revision to Diff 110954.
arphaman marked an inline comment as done.
arphaman added a comment.

Address review comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D36156

Files:
  include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  include/clang/Tooling/Refactoring/Rename/SymbolName.h
  include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
  include/clang/Tooling/Refactoring/Rename/USRLocFinder.h
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -23,6 +23,7 @@
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Core/Lookup.h"
 #include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "clang/Tooling/Refactoring/Rename/USRFinder.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -68,11 +69,9 @@
 
   // Non-visitors:
 
-  // \brief Returns a list of unique locations. Duplicate or overlapping
-  // locations are erroneous and should be reported!
-  const std::vector () const {
-return LocationsFound;
-  }
+  /// \brief Returns a set of unique symbol occurrences. Duplicate or
+  /// overlapping occurrences are erroneous and should be reported!
+  SymbolOccurrences () { return Occurrences; }
 
 private:
   void checkAndAddLocation(SourceLocation Loc) {
@@ -82,17 +81,18 @@
 StringRef TokenName =
 Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
  Context.getSourceManager(), Context.getLangOpts());
-size_t Offset = TokenName.find(PrevName);
+size_t Offset = TokenName.find(PrevName.getNamePieces()[0]);
 
 // The token of the source location we find actually has the old
 // name.
 if (Offset != StringRef::npos)
-  LocationsFound.push_back(BeginLoc.getLocWithOffset(Offset));
+  Occurrences.emplace_back(PrevName, SymbolOccurrence::MatchingSymbol,
+   BeginLoc.getLocWithOffset(Offset));
   }
 
   const std::set USRSet;
-  const std::string PrevName;
-  std::vector LocationsFound;
+  const SymbolName PrevName;
+  SymbolOccurrences Occurrences;
   const ASTContext 
 };
 
@@ -391,12 +391,11 @@
 
 } // namespace
 
-std::vector
-getLocationsOfUSRs(const std::vector , StringRef PrevName,
-   Decl *Decl) {
+SymbolOccurrences getOccurrencesOfUSRs(ArrayRef USRs,
+   StringRef PrevName, Decl *Decl) {
   USRLocFindingASTVisitor Visitor(USRs, PrevName, Decl->getASTContext());
   Visitor.TraverseDecl(Decl);
-  return Visitor.getLocationsFound();
+  return std::move(Visitor.getOccurrences());
 }
 
 std::vector
Index: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- /dev/null
+++ lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -0,0 +1,37 @@
+//===--- SymbolOccurrences.cpp - Clang refactoring library ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang;
+using namespace tooling;
+
+SymbolOccurrence::SymbolOccurrence(const SymbolName , OccurrenceKind Kind,
+   ArrayRef Locations)
+: Kind(Kind) {
+  ArrayRef NamePieces = Name.getNamePieces();
+  assert(Locations.size() == NamePieces.size() &&
+ "mismatching number of locations and lengths");
+  assert(!Locations.empty() && "no locations");
+  if (Locations.size() == 1) {
+RangeOrNumRanges = SourceRange(
+Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
+return;
+  }
+  MultipleRanges = llvm::make_unique(Locations.size());
+  RangeOrNumRanges.setBegin(
+  SourceLocation::getFromRawEncoding(Locations.size()));
+  for (const auto  : llvm::enumerate(Locations)) {
+MultipleRanges[Loc.index()] = SourceRange(
+Loc.value(),
+Loc.value().getLocWithOffset(NamePieces[Loc.index()].size()));
+  }
+}
Index: lib/Tooling/Refactoring/Rename/RenamingAction.cpp
===
--- lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -24,14 +24,54 @@
 #include "clang/Tooling/Refactoring.h"
 #include

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Alex Lorenz via Phabricator via cfe-commits 
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 , OccurrenceKind Kind,
+   ArrayRef 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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Haojian Wu via Phabricator via cfe-commits 
hokein added a comment.

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




Comment at: include/clang/Tooling/Refactoring/Rename/SymbolName.h:19
+namespace clang {
+namespace tooling {
+

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? 



Comment at: include/clang/Tooling/Refactoring/Rename/SymbolName.h:21
+
+class SymbolName;
+

I think this can be removed?



Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:60
+static void
+applyChanges(ArrayRef AtomicChanges,
+ std::map ) {

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?



Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:61
+applyChanges(ArrayRef AtomicChanges,
+ std::map ) {
+  for (const auto AtomicChange : AtomicChanges) {

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



Comment at: lib/Tooling/Refactoring/Rename/RenamingAction.cpp:62
+ std::map ) {
+  for (const auto AtomicChange : AtomicChanges) {
+for (const auto  : AtomicChange.getReplacements()) {

nit: `const auto &`



Comment at: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp:17
+
+SymbolOccurrence::SymbolOccurrence(const SymbolName , OccurrenceKind Kind,
+   ArrayRef Locations)

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



Comment at: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp:398
   Visitor.TraverseDecl(Decl);
-  return Visitor.getLocationsFound();
+  return std::move(Visitor.getOccurrences());
 }

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


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-14 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added a comment.

Ping.


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-02 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman updated this revision to Diff 109326.
arphaman added a comment.

Address review comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D36156

Files:
  include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  include/clang/Tooling/Refactoring/Rename/SymbolName.h
  include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
  include/clang/Tooling/Refactoring/Rename/USRLocFinder.h
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -23,6 +23,7 @@
 #include "clang/Lex/Lexer.h"
 #include "clang/Tooling/Core/Lookup.h"
 #include "clang/Tooling/Refactoring/RecursiveSymbolVisitor.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
 #include "clang/Tooling/Refactoring/Rename/USRFinder.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Casting.h"
@@ -68,11 +69,9 @@
 
   // Non-visitors:
 
-  // \brief Returns a list of unique locations. Duplicate or overlapping
-  // locations are erroneous and should be reported!
-  const std::vector () const {
-return LocationsFound;
-  }
+  /// \brief Returns a set of unique symbol occurrences. Duplicate or
+  /// overlapping occurrences are erroneous and should be reported!
+  SymbolOccurrences () { return Occurrences; }
 
 private:
   void checkAndAddLocation(SourceLocation Loc) {
@@ -82,17 +81,18 @@
 StringRef TokenName =
 Lexer::getSourceText(CharSourceRange::getTokenRange(BeginLoc, EndLoc),
  Context.getSourceManager(), Context.getLangOpts());
-size_t Offset = TokenName.find(PrevName);
+size_t Offset = TokenName.find(PrevName.getNamePieces()[0]);
 
 // The token of the source location we find actually has the old
 // name.
 if (Offset != StringRef::npos)
-  LocationsFound.push_back(BeginLoc.getLocWithOffset(Offset));
+  Occurrences.emplace_back(PrevName, SymbolOccurrence::MatchingSymbol,
+   BeginLoc.getLocWithOffset(Offset));
   }
 
   const std::set USRSet;
-  const std::string PrevName;
-  std::vector LocationsFound;
+  const SymbolName PrevName;
+  SymbolOccurrences Occurrences;
   const ASTContext 
 };
 
@@ -391,12 +391,11 @@
 
 } // namespace
 
-std::vector
-getLocationsOfUSRs(const std::vector , StringRef PrevName,
-   Decl *Decl) {
+SymbolOccurrences getOccurrencesOfUSRs(ArrayRef USRs,
+   StringRef PrevName, Decl *Decl) {
   USRLocFindingASTVisitor Visitor(USRs, PrevName, Decl->getASTContext());
   Visitor.TraverseDecl(Decl);
-  return Visitor.getLocationsFound();
+  return std::move(Visitor.getOccurrences());
 }
 
 std::vector
Index: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- /dev/null
+++ lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -0,0 +1,37 @@
+//===--- SymbolOccurrences.cpp - Clang refactoring library ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h"
+#include "clang/Tooling/Refactoring/Rename/SymbolName.h"
+#include "llvm/ADT/STLExtras.h"
+
+using namespace clang;
+using namespace tooling;
+
+SymbolOccurrence::SymbolOccurrence(const SymbolName , OccurrenceKind Kind,
+   ArrayRef Locations)
+: Kind(Kind) {
+  ArrayRef NamePieces = Name.getNamePieces();
+  assert(Locations.size() == NamePieces.size() &&
+ "mismatching number of locations and lengths");
+  assert(!Locations.empty() && "no locations");
+  if (Locations.size() == 1) {
+RangeOrNumRanges = SourceRange(
+Locations[0], Locations[0].getLocWithOffset(NamePieces[0].size()));
+return;
+  }
+  MultipleRanges = llvm::make_unique(Locations.size());
+  RangeOrNumRanges.setBegin(
+  SourceLocation::getFromRawEncoding(Locations.size()));
+  for (const auto  : llvm::enumerate(Locations)) {
+MultipleRanges[Loc.index()] = SourceRange(
+Loc.value(),
+Loc.value().getLocWithOffset(NamePieces[Loc.index()].size()));
+  }
+}
Index: lib/Tooling/Refactoring/Rename/RenamingAction.cpp
===
--- lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -24,14 +24,52 @@
 #include "clang/Tooling/Refactoring.h"
 #include "clang/Tooling/Refactoring/Rename/USRLocFinder.h"

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-02 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added inline comments.



Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:65-68
+  ArrayRef getNameLocations() const { return Locations; }
+  ArrayRef getNameLengths() const {
+return llvm::makeArrayRef(NameLengths, Locations.size());
+  }

arphaman wrote:
> klimek wrote:
> > Any reason not to return ranges instead here?
> 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?
Oops, this is embarrassing! I didn't realize that `AtomicChange` had an 
additional overload that accepted `CharSourceRange`. I'll go back to ranges.


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-02 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added inline comments.



Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:65-68
+  ArrayRef getNameLocations() const { return Locations; }
+  ArrayRef getNameLengths() const {
+return llvm::makeArrayRef(NameLengths, Locations.size());
+  }

klimek wrote:
> Any reason not to return ranges instead here?
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?



Comment at: include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h:72-73
+  OccurrenceKind Kind;
+  ArrayRef Locations;
+  const unsigned *NameLengths;
+};

klimek wrote:
> Can we store ranges instead?
We could, but see above for my current reasoning.


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-02 Thread 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 getNameLocations() const { return Locations; }
+  ArrayRef 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 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-02 Thread Kim Gräsman via Phabricator via cfe-commits 
kimgr added a comment.

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.


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-01 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman added a comment.

In https://reviews.llvm.org/D36156#827733, @kimgr wrote:

> 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"?


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-01 Thread Kim Gräsman via Phabricator via cfe-commits 
kimgr added a comment.

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"?


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

[PATCH] D36156: [rename] Introduce symbol occurrences

2017-08-01 Thread Alex Lorenz via Phabricator via cfe-commits 
arphaman created this revision.
Herald added a subscriber: mgorny.

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.


Repository:
  rL LLVM

https://reviews.llvm.org/D36156

Files:
  include/clang/Tooling/Refactoring/Rename/RenamingAction.h
  include/clang/Tooling/Refactoring/Rename/SymbolOccurrences.h
  include/clang/Tooling/Refactoring/Rename/USRLocFinder.h
  lib/Tooling/Refactoring/CMakeLists.txt
  lib/Tooling/Refactoring/Rename/RenamingAction.cpp
  lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
  lib/Tooling/Refactoring/Rename/USRLocFinder.cpp

Index: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
===
--- lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
+++ lib/Tooling/Refactoring/Rename/USRLocFinder.cpp
@@ -48,8 +48,8 @@
const ASTContext )
   : RecursiveSymbolVisitor(Context.getSourceManager(),
Context.getLangOpts()),
-USRSet(USRs.begin(), USRs.end()), PrevName(PrevName), Context(Context) {
-  }
+USRSet(USRs.begin(), USRs.end()), PrevName(PrevName),
+Occurrences(PrevName), Context(Context) {}
 
   bool visitSymbolOccurrence(const NamedDecl *ND,
  ArrayRef NameRanges) {
@@ -68,11 +68,9 @@
 
   // Non-visitors:
 
-  // \brief Returns a list of unique locations. Duplicate or overlapping
-  // locations are erroneous and should be reported!
-  const std::vector () const {
-return LocationsFound;
-  }
+  /// \brief Returns a set of unique symbol occurrences. Duplicate or
+  /// overlapping occurrences are erroneous and should be reported!
+  SymbolOccurrences () { return Occurrences; }
 
 private:
   void checkAndAddLocation(SourceLocation Loc) {
@@ -87,12 +85,13 @@
 // The token of the source location we find actually has the old
 // name.
 if (Offset != StringRef::npos)
-  LocationsFound.push_back(BeginLoc.getLocWithOffset(Offset));
+  Occurrences.emplace_back(SymbolOccurrence::MatchingSymbol,
+   BeginLoc.getLocWithOffset(Offset));
   }
 
   const std::set USRSet;
   const std::string PrevName;
-  std::vector LocationsFound;
+  SymbolOccurrences Occurrences;
   const ASTContext 
 };
 
@@ -391,12 +390,11 @@
 
 } // namespace
 
-std::vector
-getLocationsOfUSRs(const std::vector , StringRef PrevName,
-   Decl *Decl) {
+SymbolOccurrences getOccurrencesOfUSRs(ArrayRef USRs,
+   StringRef PrevName, Decl *Decl) {
   USRLocFindingASTVisitor Visitor(USRs, PrevName, Decl->getASTContext());
   Visitor.TraverseDecl(Decl);
-  return Visitor.getLocationsFound();
+  return std::move(Visitor.getOccurrences());
 }
 
 std::vector
Index: lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
===
--- /dev/null
+++ lib/Tooling/Refactoring/Rename/SymbolOccurrences.cpp
@@ -0,0 +1,22 @@
+//===--- SymbolOccurrences.cpp - Clang refactoring library ===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "clang/Tooling/Refactoring/Rename/SymbolOccurrences.h"
+#include "clang/Tooling/Refactoring/AtomicChange.h"
+
+using namespace clang;
+using namespace tooling;
+
+void SymbolOccurrences::emplace_back(SymbolOccurrence::OccurrenceKind Kind,
+ ArrayRef Locations) {
+  unsigned Offset = this->Locations.size();
+  Occurrences.push_back({Kind, Offset, (unsigned)Locations.size()});
+  for (const auto  : Locations)
+this->Locations.push_back(Loc);
+}
Index: lib/Tooling/Refactoring/Rename/RenamingAction.cpp
===
--- lib/Tooling/Refactoring/Rename/RenamingAction.cpp
+++ lib/Tooling/Refactoring/Rename/RenamingAction.cpp
@@ -32,6 +32,42 @@
 namespace clang {
 namespace tooling {
 
+Expected
+createRenameReplacements(const SymbolOccurrences ,
+ const SourceManager ,
+ ArrayRef NewNameStrings) {
+  // FIXME: A true local rename can use just one AtomicChange.
+  std::vector Changes;
+  for (const auto  : Occurrences) {
+ArrayRef Locs = Occurrence.getNameLocations();
+assert(NewNameStrings.size() == Locs.size() &&
+   "Mismatching number of locations and name pieces");