[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf39bdb49086: [clangd] Implement Decl canonicalization rules 
for rename (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,58 @@
   return OtherFile;
 }
 
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Template's canonical decl is the templated declaration (i.e.
+//   ClassTemplateDecl is canonicalized to its child CXXRecordDecl,
+//   FunctionTemplateDecl - to child FunctionDecl)
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl because we want to rename both type name and its ctor/dtor.
+// - All specializations are canonicalized to the primary template. For example:
+//
+//template 
+//bool Foo = true; (1)
+//
+//template 
+//bool Foo = true; (2)
+//
+//template <>
+//bool Foo = true; (3)
+//
+// Here, both partial (2) and full (3) specializations are canonicalized to (1)
+// which ensures all three of them are renamed.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  if (const auto *VarTemplate = dyn_cast(D))
+return canonicalRenameDecl(
+VarTemplate->getSpecializedTemplate()->getTemplatedDecl());
+  if (const auto *Template = dyn_cast(D))
+if (const NamedDecl *TemplatedDecl = Template->getTemplatedDecl())
+  return canonicalRenameDecl(TemplatedDecl);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+return canonicalRenameDecl(
+ClassTemplateSpecialization->getSpecializedTemplate()
+->getTemplatedDecl());
+  if (const auto *Method = dyn_cast(D)) {
+if (Method->getDeclKind() == Decl::Kind::CXXConstructor ||
+Method->getDeclKind() == Decl::Kind::CXXDestructor)
+  return canonicalRenameDecl(Method->getParent());
+if (const FunctionDecl *InstantiatedMethod =
+Method->getInstantiatedFromMemberFunction())
+  Method = cast(InstantiatedMethod);
+// FIXME(kirillbobyrev): For virtual methods with
+// size_overridden_methods() > 1, this will not rename all functions it
+// overrides, because this code assumes there is a single canonical
+// declaration.
+while (Method->isVirtual() && Method->size_overridden_methods())
+  Method = *Method->overridden_methods().begin();
+return dyn_cast(Method->getCanonicalDecl());
+  }
+  if (const auto *Function = dyn_cast(D))
+if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
+  return canonicalRenameDecl(Template);
+  return dyn_cast(D->getCanonicalDecl());
+}
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +142,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -226,19 +275,8 @@
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  assert(canonicalRenameDecl() ==  &&
+ "ND should be already canonicalized.");
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
@@ -246,11 +284,11 @@
   if (Ref.Targets.empty())
 return;
   for 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307013.
kbobyrev marked an inline comment as done.
kbobyrev added a comment.

Get rid of forward declaration.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,58 @@
   return OtherFile;
 }
 
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Template's canonical decl is the templated declaration (i.e.
+//   ClassTemplateDecl is canonicalized to its child CXXRecordDecl,
+//   FunctionTemplateDecl - to child FunctionDecl)
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl because we want to rename both type name and its ctor/dtor.
+// - All specializations are canonicalized to the primary template. For example:
+//
+//template 
+//bool Foo = true; (1)
+//
+//template 
+//bool Foo = true; (2)
+//
+//template <>
+//bool Foo = true; (3)
+//
+// Here, both partial (2) and full (3) specializations are canonicalized to (1)
+// which ensures all three of them are renamed.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  if (const auto *VarTemplate = dyn_cast(D))
+return canonicalRenameDecl(
+VarTemplate->getSpecializedTemplate()->getTemplatedDecl());
+  if (const auto *Template = dyn_cast(D))
+if (const NamedDecl *TemplatedDecl = Template->getTemplatedDecl())
+  return canonicalRenameDecl(TemplatedDecl);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+return canonicalRenameDecl(
+ClassTemplateSpecialization->getSpecializedTemplate()
+->getTemplatedDecl());
+  if (const auto *Method = dyn_cast(D)) {
+if (Method->getDeclKind() == Decl::Kind::CXXConstructor ||
+Method->getDeclKind() == Decl::Kind::CXXDestructor)
+  return canonicalRenameDecl(Method->getParent());
+if (const FunctionDecl *InstantiatedMethod =
+Method->getInstantiatedFromMemberFunction())
+  Method = cast(InstantiatedMethod);
+// FIXME(kirillbobyrev): For virtual methods with
+// size_overridden_methods() > 1, this will not rename all functions it
+// overrides, because this code assumes there is a single canonical
+// declaration.
+while (Method->isVirtual() && Method->size_overridden_methods())
+  Method = *Method->overridden_methods().begin();
+return dyn_cast(Method->getCanonicalDecl());
+  }
+  if (const auto *Function = dyn_cast(D))
+if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
+  return canonicalRenameDecl(Template);
+  return dyn_cast(D->getCanonicalDecl());
+}
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +142,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -226,19 +275,8 @@
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  assert(canonicalRenameDecl() ==  &&
+ "ND should be already canonicalized.");
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
@@ -246,11 +284,11 @@
   if (Ref.Targets.empty())
 return;
   for (const auto *Target : Ref.Targets) {
-auto ID = getSymbolID(Target);
-if (!ID || 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:78
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+

nit: maybe we can just move the function definition to here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks, this looks great now.

I think we can get rid of the `clangToolingRefactoring` dependence from clangd 
CMake files, clangd now should have no use of any functions there, could you 
take a look?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 306988.
kbobyrev added a comment.

Make use of TemplatedDecl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +92,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +221,64 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Template's canonical decl is the templated declaration (i.e.
+//   ClassTemplateDecl is canonicalized to its child CXXRecordDecl,
+//   FunctionTemplateDecl - to child FunctionDecl)
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl because we want to rename both type name and its ctor/dtor.
+// - All specializations are canonicalized to the primary template. For example:
+//
+//template 
+//bool Foo = true; (1)
+//
+//template 
+//bool Foo = true; (2)
+//
+//template <>
+//bool Foo = true; (3)
+//
+// Here, both partial (2) and full (3) specializations are canonicalized to (1)
+// which ensures all three of them are renamed.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  if (const auto *VarTemplate = dyn_cast(D))
+return canonicalRenameDecl(
+VarTemplate->getSpecializedTemplate()->getTemplatedDecl());
+  if (const auto *Template = dyn_cast(D))
+if (const NamedDecl *TemplatedDecl = Template->getTemplatedDecl())
+  return canonicalRenameDecl(TemplatedDecl);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+return canonicalRenameDecl(
+ClassTemplateSpecialization->getSpecializedTemplate()
+->getTemplatedDecl());
+  if (const auto *Method = dyn_cast(D)) {
+if (Method->getDeclKind() == Decl::Kind::CXXConstructor ||
+Method->getDeclKind() == Decl::Kind::CXXDestructor)
+  return canonicalRenameDecl(Method->getParent());
+if (const FunctionDecl *InstantiatedMethod =
+Method->getInstantiatedFromMemberFunction())
+  Method = cast(InstantiatedMethod);
+// FIXME(kirillbobyrev): For virtual methods with
+// size_overridden_methods() > 1, this will not rename all functions it
+// overrides, because this code assumes there is a single canonical
+// declaration.
+while (Method->isVirtual() && Method->size_overridden_methods())
+  Method = *Method->overridden_methods().begin();
+return dyn_cast(Method->getCanonicalDecl());
+  }
+  if (const auto *Function = dyn_cast(D))
+if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
+  return canonicalRenameDecl(Template);
+  return dyn_cast(D->getCanonicalDecl());
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  assert(canonicalRenameDecl() ==  &&
+ "ND should be already canonicalized.");
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
@@ -246,11 +286,11 @@
   if 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 306987.
kbobyrev marked 4 inline comments as done.
kbobyrev added a comment.

Resolve comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +92,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +221,64 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Template's canonical decl is the templated declaration (i.e.
+//   ClassTemplateDecl is canonicalized to its child CXXRecordDecl,
+//   FunctionTemplateDecl - to child FunctionDecl)
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl because we want to rename both type name and its ctor/dtor.
+// - All specializations are canonicalized to the primary template. For example:
+//
+//template 
+//bool Foo = true; (1)
+//
+//template 
+//bool Foo = true; (2)
+//
+//template <>
+//bool Foo = true; (3)
+//
+// Here, both partial (2) and full (3) specializations are canonicalized to (1)
+// which ensures all three of them are renamed.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  if (const auto *VarTemplate = dyn_cast(D))
+return canonicalRenameDecl(
+VarTemplate->getSpecializedTemplate()->getTemplatedDecl());
+  if (const auto *Template = dyn_cast(D))
+if (const NamedDecl *TemplatedDecl = Template->getTemplatedDecl())
+  return canonicalRenameDecl(Template->getTemplatedDecl());
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+return canonicalRenameDecl(
+ClassTemplateSpecialization->getSpecializedTemplate()
+->getTemplatedDecl());
+  if (const auto *Method = dyn_cast(D)) {
+if (Method->getDeclKind() == Decl::Kind::CXXConstructor ||
+Method->getDeclKind() == Decl::Kind::CXXDestructor)
+  return canonicalRenameDecl(Method->getParent());
+if (const FunctionDecl *InstantiatedMethod =
+Method->getInstantiatedFromMemberFunction())
+  Method = cast(InstantiatedMethod);
+// FIXME(kirillbobyrev): For virtual methods with
+// size_overridden_methods() > 1, this will not rename all functions it
+// overrides, because this code assumes there is a single canonical
+// declaration.
+while (Method->isVirtual() && Method->size_overridden_methods())
+  Method = *Method->overridden_methods().begin();
+return dyn_cast(Method->getCanonicalDecl());
+  }
+  if (const auto *Function = dyn_cast(D))
+if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
+  return canonicalRenameDecl(Template);
+  return dyn_cast(D->getCanonicalDecl());
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  assert(canonicalRenameDecl() ==  &&
+ "ND should be already canonicalized.");
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

Thanks, this looks nicer now.




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:247
+  if (const auto *Destructor = dyn_cast(D))
+return canonicalRenameDecl(Destructor->getParent());
+  if (const auto *VarTemplate = dyn_cast(D))

ctor & dtor is one of the kinds of `CXXMethodDecl`, I think we can merge the 
ctor & dtor cases into the `if (const auto *Method = 
dyn_cast(D)) {`. 



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:257
+->getTemplatedDecl();
+  if (const auto *Method = dyn_cast(D)) {
+if (const FunctionDecl *InstantiatedMethod =

using `else if` would be more clear  -- as now we modify `D` is the preceding 
`if`, if the D is assigned to a CXXMethodDecl in the `preceding` if, then it 
may fall into this branch.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:262
+while (Method->isVirtual() && Method->size_overridden_methods())
+  Method = *Method->overridden_methods().begin();
+D = Method;

oh, it is a tricky case where a method overrides > 1 virtual methods. Looks 
like we will regress this case in this patch, e.g.

```
class Foo {
  virtual void foo();
};

class Bar {
  virtual void foo();
};

class C : public Foo, Bar {
  void foo() override; // -> rename foo => bar
};
```

prior to the patch, both `Foo::foo` and `Bar::foo` will be renamed, after this 
patch, only one of them will be renamed (with within-file rename)? I think it 
is ok, but probably need some comments here.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:265
+  }
+  if (const auto *Function = dyn_cast(D))
+if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())

the same, `else`

IIUC, `CXXMethodDecl` must be processed before `FunctionDecl`, I think we can 
add an assertion (the FunctionDecl must not be `CXXMethodDecl`).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 306334.
kbobyrev marked 8 inline comments as done.
kbobyrev added a comment.

Resolve review comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +92,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +221,59 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Template's canonical decl is the templated declaration (i.e.
+//   ClassTemplateDecl is canonicalized to its child CXXRecordDecl,
+//   FunctionTemplateDecl - to child FunctionDecl)
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl because we want to rename both type name and its ctor/dtor.
+// - All specializations are canonicalized to the primary template. For example:
+//
+//template 
+//bool Foo = true; (1)
+//
+//template 
+//bool Foo = true; (2)
+//
+//template <>
+//bool Foo = true; (3)
+//
+// Here, both partial (2) and full (3) specializations are canonicalized to (1)
+// which ensures all three of them are renamed.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  if (const auto *Constructor = dyn_cast(D))
+return canonicalRenameDecl(Constructor->getParent());
+  if (const auto *Destructor = dyn_cast(D))
+return canonicalRenameDecl(Destructor->getParent());
+  if (const auto *VarTemplate = dyn_cast(D))
+D = VarTemplate->getSpecializedTemplate()->getTemplatedDecl();
+  if (const auto *Template = dyn_cast(D))
+if (const NamedDecl *TemplatedDecl = Template->getTemplatedDecl())
+  return canonicalRenameDecl(Template->getTemplatedDecl());
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+D = ClassTemplateSpecialization->getSpecializedTemplate()
+->getTemplatedDecl();
+  if (const auto *Method = dyn_cast(D)) {
+if (const FunctionDecl *InstantiatedMethod =
+Method->getInstantiatedFromMemberFunction())
+  Method = cast(InstantiatedMethod);
+while (Method->isVirtual() && Method->size_overridden_methods())
+  Method = *Method->overridden_methods().begin();
+D = Method;
+  }
+  if (const auto *Function = dyn_cast(D))
+if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
+  return canonicalRenameDecl(Template);
+  return dyn_cast(D->getCanonicalDecl());
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  assert(canonicalRenameDecl() ==  &&
+ "ND should be already canonicalized.");
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
@@ -246,11 +281,11 @@
   if (Ref.Targets.empty())
 return;
   for (const auto *Target : Ref.Targets) {
-auto ID = getSymbolID(Target);
-if (!ID || TargetIDs.find(ID) == TargetIDs.end())
+if (canonicalRenameDecl(Target) == ) {
+  

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Need to address the comments.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:274
+return canonicalRenameDecl(Function);
+  return D;
+}

I think we should call `getCanonicalDecl` before returning the final result 
(calling it at the beginning doesn't guarantee the final result is canonical).



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:601
 return makeError(ReasonToReject::AmbiguousSymbol);
-  const auto  =
-  llvm::cast(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
+  const auto  = llvm::cast(*(*DeclsUnderCursor.begin()));
   if (RenameDecl.getName() == RInputs.NewName)

nit: llvm::cast is not needed, just `const NamedDecl  = 
*(*DeclsUnderCursor.begin());`



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:239
+  if (const auto *Template = D->getPrimaryTemplate())
+return canonicalRenameDecl(Template);
+  return D;

kbobyrev wrote:
> hokein wrote:
> > the `auto` + varies `canonicalRenameDecl` overrides make the code hard to 
> > follow.
> > 
> > since these functions are small, I think we can inline them into the main 
> > `canonicalRenameDecl(const NamedDecl *D)`
> I removed `auto`s but I believe merging them into a single function would not 
> be great for two reasons:
> 
> * Some classes are already handled by the same function outside of the main 
> one: ctor/dtor's parent `CXXRecordDecl`, etc: in the main function we'd have 
> to have a weird logic behind extracting those and this is likely to result in 
> more code
> * Some functions call other ones (e.g. here we have 
> `canonicalRenameDecl(Template)` - sure, right now it's just 
> `D->getTemplatedDecl()` but if we decide to change something and when we 
> introduce more code it'd be easy to forget and code duplication is not really 
> a good idea).
> 
> WDYT?
this is more about encapsulation -- my understanding is that 
`canonicalRenameDecl(const NamedDecl)` should be the only main entry, others 
are just implementation details, so we expect client just calls the main 
function, but not other overrides.  The current state leaks these 
implementation details outside of the world.


> Some classes are already handled by the same function outside of the main 
> one: ctor/dtor's parent CXXRecordDecl, etc: in the main function we'd have to 
> have a weird logic behind extracting those and this is likely to result in 
> more code

don't follow this, for ctor/dtor's parent CXXRecordDecl, I think just calling 
the main canonicalRenameDecl recursively on the parent should be enough?


> Some functions call other ones (e.g. here we have 
> canonicalRenameDecl(Template) - sure, right now it's just 
> D->getTemplatedDecl() but if we decide to change something and when we 
> introduce more code it'd be easy to forget and code duplication is not really 
> a good idea).

I agree your points. Given the current code, I think inlining them seems quite 
straight-forward and clearer to me (the only case is `FunctionDecl`, which 
calls the `canonicalRenameDecl(const TemplateDecl *D)`, but we can just call 
`getTemplatedDecl` as well). 

Regarding your concerns, I think we can always find ways to extend the code 
when we need to enhance the TemplateDecl case (what kind of enhancement we'll 
do for templateDecl?)




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) 
{
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}

kbobyrev wrote:
> hokein wrote:
> > want to confirm my understanding:
> > 
> > given the example below:
> > 
> > ```
> > template
> > class Foo {};
> > 
> > template<>
> > class Foo {};
> > ```
> > 
> > the AST is like:
> > 
> > ```
> > ClassTemplateDecl
> >   |-CXXRecordDecl (Foo definition) -> (1)
> >   |-ClassTemplateSpecialization. 
> > 
> > ClassTemplateSpecializationDecl -> call canonicalRenameDecl on it.
> >   |-Template Argument int
> >   |-CXXRecordDecl -> (2)
> > ```
> > 
> > if we pass `ClassTemplateSpecializationDecl` to this function, this 
> > function will return (2)?
> No, this will return (1): `getSpecializedTemplate()` returns 
> `ClassTemplateDecl` and then `getTemplatedDecl()` gets to `CXXRecordDecl` in 
> it.
I see. thanks for the explanation. I misunderstood the `getSpecializedTemplate`.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:261
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.

kbobyrev wrote:
> hokein wrote:
> > I think the motivation is for merely renaming the explicit template 
> > specialization, but not the primary template?
> How come? The specialized template is the primary template, am I missing 
> something?
ah, you're right, sorry -- I was confused by the term "specialized template", I 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 306027.
kbobyrev added a comment.

Remove leftover test.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +92,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +221,65 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const NamedDecl *canonicalRenameDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const CXXMethodDecl *D) {
+  const CXXMethodDecl *Result = D;
+  if (const FunctionDecl *InstantiatedMethod =
+  D->getInstantiatedFromMemberFunction())
+Result = cast(InstantiatedMethod);
+  while (Result->isVirtual() && Result->size_overridden_methods())
+Result = *Result->overridden_methods().begin();
+  return Result;
+}
+
+const NamedDecl *canonicalRenameDecl(const FunctionDecl *D) {
+  if (const FunctionTemplateDecl *Template = D->getPrimaryTemplate())
+return canonicalRenameDecl(Template);
+  return D;
+}
+
+const NamedDecl *canonicalRenameDecl(const VarTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  D = dyn_cast(D->getCanonicalDecl());
+  if (const auto *Constructor = dyn_cast(D))
+return canonicalRenameDecl(Constructor->getParent());
+  if (const auto *Destructor = dyn_cast(D))
+return canonicalRenameDecl(Destructor->getParent());
+  if (const auto *VarTemplate = dyn_cast(D))
+return canonicalRenameDecl(VarTemplate);
+  if (const auto *Template = dyn_cast(D))
+return canonicalRenameDecl(Template);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+return canonicalRenameDecl(ClassTemplateSpecialization);
+  if (const auto *Method = dyn_cast(D))
+return canonicalRenameDecl(Method);
+  if (const auto *Function = dyn_cast(D))
+return canonicalRenameDecl(Function);
+  return D;
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  assert(canonicalRenameDecl() ==  &&
+ "ND should be already canonicalized.");
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
@@ -246,11 +287,11 @@
   if (Ref.Targets.empty())
 return;
   for (const auto *Target : Ref.Targets) {
-auto ID = getSymbolID(Target);
-if (!ID || TargetIDs.find(ID) == TargetIDs.end())
+if (canonicalRenameDecl(Target) == ) {
+  Results.push_back(Ref.NameLoc);
   return;
+

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:239
+  if (const auto *Template = D->getPrimaryTemplate())
+return canonicalRenameDecl(Template);
+  return D;

hokein wrote:
> the `auto` + varies `canonicalRenameDecl` overrides make the code hard to 
> follow.
> 
> since these functions are small, I think we can inline them into the main 
> `canonicalRenameDecl(const NamedDecl *D)`
I removed `auto`s but I believe merging them into a single function would not 
be great for two reasons:

* Some classes are already handled by the same function outside of the main 
one: ctor/dtor's parent `CXXRecordDecl`, etc: in the main function we'd have to 
have a weird logic behind extracting those and this is likely to result in more 
code
* Some functions call other ones (e.g. here we have 
`canonicalRenameDecl(Template)` - sure, right now it's just 
`D->getTemplatedDecl()` but if we decide to change something and when we 
introduce more code it'd be easy to forget and code duplication is not really a 
good idea).

WDYT?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:246
+  if (const auto *Primary = TemplatedDecl->getPrimaryTemplate())
+return Primary;
+  return TemplatedDecl;

hokein wrote:
> didn't quite follow the code here, the code looks like we get the 
> FunctionTemplateDecl back via the code path (FunctionTemplateDecl -> 
> FunctionDecl -> FunctionTemplateDecl), and it looks like we're not using the 
> specialized declaration as the canonical decl.
Uh, good catch, thanks. This one is simply not needed because we handle 
`TemplateDecl` which does the right thing for `FunctionTemplateDecl`.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) 
{
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}

hokein wrote:
> want to confirm my understanding:
> 
> given the example below:
> 
> ```
> template
> class Foo {};
> 
> template<>
> class Foo {};
> ```
> 
> the AST is like:
> 
> ```
> ClassTemplateDecl
>   |-CXXRecordDecl (Foo definition) -> (1)
>   |-ClassTemplateSpecialization. 
> 
> ClassTemplateSpecializationDecl -> call canonicalRenameDecl on it.
>   |-Template Argument int
>   |-CXXRecordDecl -> (2)
> ```
> 
> if we pass `ClassTemplateSpecializationDecl` to this function, this function 
> will return (2)?
No, this will return (1): `getSpecializedTemplate()` returns 
`ClassTemplateDecl` and then `getTemplatedDecl()` gets to `CXXRecordDecl` in it.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:261
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.

hokein wrote:
> I think the motivation is for merely renaming the explicit template 
> specialization, but not the primary template?
How come? The specialized template is the primary template, am I missing 
something?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 306026.
kbobyrev marked 2 inline comments as done.
kbobyrev added a comment.

Resolve some comments, simplify the code.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -94,6 +94,15 @@
   // For each "^" this test moves cursor to its location and applies renaming
   // while checking that all identifiers in [[]] ranges are also renamed.
   llvm::StringRef Tests[] = {
+  // Example.
+  R"cpp(
+template
+void [[f^oo]]();
+
+template<>
+void [[f^oo]]();
+  )cpp",
+
   // Function.
   R"cpp(
 void [[foo^]]() {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +92,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +221,65 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const NamedDecl *canonicalRenameDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const CXXMethodDecl *D) {
+  const CXXMethodDecl *Result = D;
+  if (const FunctionDecl *InstantiatedMethod =
+  D->getInstantiatedFromMemberFunction())
+Result = cast(InstantiatedMethod);
+  while (Result->isVirtual() && Result->size_overridden_methods())
+Result = *Result->overridden_methods().begin();
+  return Result;
+}
+
+const NamedDecl *canonicalRenameDecl(const FunctionDecl *D) {
+  if (const FunctionTemplateDecl *Template = D->getPrimaryTemplate())
+return canonicalRenameDecl(Template);
+  return D;
+}
+
+const NamedDecl *canonicalRenameDecl(const VarTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  D = dyn_cast(D->getCanonicalDecl());
+  if (const auto *Constructor = dyn_cast(D))
+return canonicalRenameDecl(Constructor->getParent());
+  if (const auto *Destructor = dyn_cast(D))
+return canonicalRenameDecl(Destructor->getParent());
+  if (const auto *VarTemplate = dyn_cast(D))
+return canonicalRenameDecl(VarTemplate);
+  if (const auto *Template = dyn_cast(D))
+return canonicalRenameDecl(Template);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+return canonicalRenameDecl(ClassTemplateSpecialization);
+  if (const auto *Method = dyn_cast(D))
+return canonicalRenameDecl(Method);
+  if (const auto *Function = dyn_cast(D))
+return canonicalRenameDecl(Function);
+  return D;
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

didn't finish all parts (looked at the FunctionDecl and RecordDecl), I think we 
need more documentation/comments in the code to specify the canonical behavior 
(templates are tricky).




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:239
+  if (const auto *Template = D->getPrimaryTemplate())
+return canonicalRenameDecl(Template);
+  return D;

the `auto` + varies `canonicalRenameDecl` overrides make the code hard to 
follow.

since these functions are small, I think we can inline them into the main 
`canonicalRenameDecl(const NamedDecl *D)`



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:246
+  if (const auto *Primary = TemplatedDecl->getPrimaryTemplate())
+return Primary;
+  return TemplatedDecl;

didn't quite follow the code here, the code looks like we get the 
FunctionTemplateDecl back via the code path (FunctionTemplateDecl -> 
FunctionDecl -> FunctionTemplateDecl), and it looks like we're not using the 
specialized declaration as the canonical decl.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:255
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) 
{
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}

want to confirm my understanding:

given the example below:

```
template
class Foo {};

template<>
class Foo {};
```

the AST is like:

```
ClassTemplateDecl
  |-CXXRecordDecl (Foo definition) -> (1)
  |-ClassTemplateSpecialization. 

ClassTemplateSpecializationDecl -> call canonicalRenameDecl on it.
  |-Template Argument int
  |-CXXRecordDecl -> (2)
```

if we pass `ClassTemplateSpecializationDecl` to this function, this function 
will return (2)?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:261
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.

I think the motivation is for merely renaming the explicit template 
specialization, but not the primary template?



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:610
   const auto  =
   llvm::cast(*(*DeclsUnderCursor.begin())->getCanonicalDecl());
   if (RenameDecl.getName() == RInputs.NewName)

 `getCanonicalDecl` is not needed now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304893.
kbobyrev added a comment.

Remove unused header (was added for llvm_unreachable).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +92,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +221,73 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const NamedDecl *canonicalRenameDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const CXXMethodDecl *D) {
+  const auto *Result = D;
+  if (const auto *InstantiatedMethod = D->getInstantiatedFromMemberFunction())
+Result = cast(InstantiatedMethod);
+  while (Result->isVirtual() && Result->size_overridden_methods())
+Result = *Result->overridden_methods().begin();
+  return Result;
+}
+
+const NamedDecl *canonicalRenameDecl(const FunctionDecl *D) {
+  if (const auto *Template = D->getPrimaryTemplate())
+return canonicalRenameDecl(Template);
+  return D;
+}
+
+const NamedDecl *canonicalRenameDecl(const FunctionTemplateDecl *D) {
+  const auto *TemplatedDecl = D->getTemplatedDecl();
+  if (const auto *Primary = TemplatedDecl->getPrimaryTemplate())
+return Primary;
+  return TemplatedDecl;
+}
+
+const NamedDecl *canonicalRenameDecl(const VarTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  D = dyn_cast(D->getCanonicalDecl());
+  if (const auto *Constructor = dyn_cast(D))
+return canonicalRenameDecl(Constructor->getParent());
+  if (const auto *Destructor = dyn_cast(D))
+return canonicalRenameDecl(Destructor->getParent());
+  if (const auto *FuctionTemplate = dyn_cast(D))
+return canonicalRenameDecl(FuctionTemplate);
+  if (const auto *VarTemplate = dyn_cast(D))
+return canonicalRenameDecl(VarTemplate);
+  if (const auto *Template = dyn_cast(D))
+return canonicalRenameDecl(Template);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+return canonicalRenameDecl(ClassTemplateSpecialization);
+  if (const auto *Method = dyn_cast(D))
+return canonicalRenameDecl(Method);
+  if (const auto *Function = dyn_cast(D))
+return canonicalRenameDecl(Function);
+  return D;
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  assert(canonicalRenameDecl() ==  &&
+ "ND should be already canonicalized.");
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304787.
kbobyrev added a comment.

Completely remove new tests from this patch (to be provided in a different
one).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,12 +18,12 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include 
 
@@ -76,6 +76,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -91,9 +93,7 @@
   for (const NamedDecl *D :
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
-// Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +222,73 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const NamedDecl *canonicalRenameDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const CXXMethodDecl *D) {
+  const auto *Result = D;
+  if (const auto *InstantiatedMethod = D->getInstantiatedFromMemberFunction())
+Result = cast(InstantiatedMethod);
+  while (Result->isVirtual() && Result->size_overridden_methods())
+Result = *Result->overridden_methods().begin();
+  return Result;
+}
+
+const NamedDecl *canonicalRenameDecl(const FunctionDecl *D) {
+  if (const auto *Template = D->getPrimaryTemplate())
+return canonicalRenameDecl(Template);
+  return D;
+}
+
+const NamedDecl *canonicalRenameDecl(const FunctionTemplateDecl *D) {
+  const auto *TemplatedDecl = D->getTemplatedDecl();
+  if (const auto *Primary = TemplatedDecl->getPrimaryTemplate())
+return Primary;
+  return TemplatedDecl;
+}
+
+const NamedDecl *canonicalRenameDecl(const VarTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+// Canonical declarations help simplify the process of renaming. Examples:
+// - Given a constructor/destructor, canonical declaration is the parent
+//   CXXRecordDecl
+// - Specializations should point to the specialized declaration.
+// - Instantiations should point to instantiated declaration.
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D) {
+  D = dyn_cast(D->getCanonicalDecl());
+  if (const auto *Constructor = dyn_cast(D))
+return canonicalRenameDecl(Constructor->getParent());
+  if (const auto *Destructor = dyn_cast(D))
+return canonicalRenameDecl(Destructor->getParent());
+  if (const auto *FuctionTemplate = dyn_cast(D))
+return canonicalRenameDecl(FuctionTemplate);
+  if (const auto *VarTemplate = dyn_cast(D))
+return canonicalRenameDecl(VarTemplate);
+  if (const auto *Template = dyn_cast(D))
+return canonicalRenameDecl(Template);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(D))
+return canonicalRenameDecl(ClassTemplateSpecialization);
+  if (const auto *Method = dyn_cast(D))
+return canonicalRenameDecl(Method);
+  if (const auto *Function = dyn_cast(D))
+return canonicalRenameDecl(Function);
+  return D;
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304784.
kbobyrev added a comment.

Add a test with static class member.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -141,6 +141,12 @@
   ~[[F^oo]]();
   void f([[F^oo]] x);
 };
+
+template
+[[F^oo]]::[[Fo^o]]() {}
+
+template
+[[F^oo]]::~[[Fo^o]]() {}
   )cpp",
 
   // Template class constructor.
@@ -152,6 +158,9 @@
   template
   [[F^oo]](T t);
 };
+
+template
+[[F^oo]]::[[Fo^o]]() {}
   )cpp",
 
   // Class in template argument.
@@ -191,11 +200,15 @@
 struct C : B {
   void [[f^oo]]() override {}
 };
+struct D : B {
+  void [[f^oo]]() override {}
+};
 
 void func() {
   A().[[f^oo]]();
   B().[[f^oo]]();
   C().[[f^oo]]();
+  D().[[f^oo]]();
 }
   )cpp",
 
@@ -307,6 +320,19 @@
 }
   )cpp",
 
+  // Static class member.
+  R"cpp(
+struct Foo {
+  static Foo *[[Static^Member]];
+};
+
+Foo* Foo::[[Static^Member]] = nullptr;
+
+void foo() {
+  Foo* Pointer = Foo::[[Static^Member]];
+}
+  )cpp",
+
   // Reference in lambda parameters.
   R"cpp(
 template 
@@ -588,6 +614,123 @@
   ns::[[Old^Alias]] Bar;
 }
   )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Templated class specialization.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  R"cpp(
+template 
+bool [[F^oo]] = true;
+
+// Explicit template specialization
+template <>
+bool [[F^oo]] = false;
+
+// Partial template specialization
+template 
+bool [[F^oo]] = false;
+
+void foo() {
+  // Ref to the explicit template specialization
+  [[F^oo]];
+  // Ref to the primary template.
+  [[F^oo]];
+}
+  )cpp",
+  R"cpp(
+template 
+void [[f^oo]](T t);
+
+template <>
+void [[f^oo]](int a);
+
+void test() {
+  [[f^oo]](1);
+}
+  )cpp",
+
+  // User defined conversion.
+  R"cpp(
+class [[F^oo]] {
+public:
+  [[F^oo]]() {}
+};
+
+class Baz {
+public:
+  operator [[F^oo]]() {
+return [[F^oo]]();
+  }
+};
+
+int main() {
+  Baz boo;
+  [[F^oo]] foo = static_cast<[[F^oo]]>(boo);
+}
+  )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,12 +18,12 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include 
 
@@ -76,6 +76,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304785.
kbobyrev added a comment.

Remove (now) outdated comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -141,6 +141,12 @@
   ~[[F^oo]]();
   void f([[F^oo]] x);
 };
+
+template
+[[F^oo]]::[[Fo^o]]() {}
+
+template
+[[F^oo]]::~[[Fo^o]]() {}
   )cpp",
 
   // Template class constructor.
@@ -152,6 +158,9 @@
   template
   [[F^oo]](T t);
 };
+
+template
+[[F^oo]]::[[Fo^o]]() {}
   )cpp",
 
   // Class in template argument.
@@ -191,11 +200,15 @@
 struct C : B {
   void [[f^oo]]() override {}
 };
+struct D : B {
+  void [[f^oo]]() override {}
+};
 
 void func() {
   A().[[f^oo]]();
   B().[[f^oo]]();
   C().[[f^oo]]();
+  D().[[f^oo]]();
 }
   )cpp",
 
@@ -307,6 +320,19 @@
 }
   )cpp",
 
+  // Static class member.
+  R"cpp(
+struct Foo {
+  static Foo *[[Static^Member]];
+};
+
+Foo* Foo::[[Static^Member]] = nullptr;
+
+void foo() {
+  Foo* Pointer = Foo::[[Static^Member]];
+}
+  )cpp",
+
   // Reference in lambda parameters.
   R"cpp(
 template 
@@ -588,6 +614,123 @@
   ns::[[Old^Alias]] Bar;
 }
   )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Templated class specialization.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  R"cpp(
+template 
+bool [[F^oo]] = true;
+
+// Explicit template specialization
+template <>
+bool [[F^oo]] = false;
+
+// Partial template specialization
+template 
+bool [[F^oo]] = false;
+
+void foo() {
+  // Ref to the explicit template specialization
+  [[F^oo]];
+  // Ref to the primary template.
+  [[F^oo]];
+}
+  )cpp",
+  R"cpp(
+template 
+void [[f^oo]](T t);
+
+template <>
+void [[f^oo]](int a);
+
+void test() {
+  [[f^oo]](1);
+}
+  )cpp",
+
+  // User defined conversion.
+  R"cpp(
+class [[F^oo]] {
+public:
+  [[F^oo]]() {}
+};
+
+class Baz {
+public:
+  operator [[F^oo]]() {
+return [[F^oo]]();
+  }
+};
+
+int main() {
+  Baz boo;
+  [[F^oo]] foo = static_cast<[[F^oo]]>(boo);
+}
+  )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,12 +18,12 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include 
 
@@ -76,6 +76,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304783.
kbobyrev added a comment.

Add missing user-defined conversion test and a couple of missing cases with
ctor/dtor out-of-line implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -141,6 +141,12 @@
   ~[[F^oo]]();
   void f([[F^oo]] x);
 };
+
+template
+[[F^oo]]::[[Fo^o]]() {}
+
+template
+[[F^oo]]::~[[Fo^o]]() {}
   )cpp",
 
   // Template class constructor.
@@ -152,6 +158,9 @@
   template
   [[F^oo]](T t);
 };
+
+template
+[[F^oo]]::[[Fo^o]]() {}
   )cpp",
 
   // Class in template argument.
@@ -191,11 +200,15 @@
 struct C : B {
   void [[f^oo]]() override {}
 };
+struct D : B {
+  void [[f^oo]]() override {}
+};
 
 void func() {
   A().[[f^oo]]();
   B().[[f^oo]]();
   C().[[f^oo]]();
+  D().[[f^oo]]();
 }
   )cpp",
 
@@ -588,6 +601,123 @@
   ns::[[Old^Alias]] Bar;
 }
   )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Templated class specialization.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  R"cpp(
+template 
+bool [[F^oo]] = true;
+
+// Explicit template specialization
+template <>
+bool [[F^oo]] = false;
+
+// Partial template specialization
+template 
+bool [[F^oo]] = false;
+
+void foo() {
+  // Ref to the explicit template specialization
+  [[F^oo]];
+  // Ref to the primary template.
+  [[F^oo]];
+}
+  )cpp",
+  R"cpp(
+template 
+void [[f^oo]](T t);
+
+template <>
+void [[f^oo]](int a);
+
+void test() {
+  [[f^oo]](1);
+}
+  )cpp",
+
+  // User defined conversion.
+  R"cpp(
+class [[F^oo]] {
+public:
+  [[F^oo]]() {}
+};
+
+class Baz {
+public:
+  operator [[F^oo]]() {
+return [[F^oo]]();
+  }
+};
+
+int main() {
+  Baz boo;
+  [[F^oo]] foo = static_cast<[[F^oo]]>(boo);
+}
+  )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,12 +18,12 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include 
 
@@ -76,6 +76,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -92,8 +94,7 @@
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
 // Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304776.
kbobyrev marked 5 inline comments as done.
kbobyrev added a comment.

- Handle VarDecl
- Handle FunctionTemplateDecl
- Remove FieldDecl handling (leave for the future patches)
- Simplify code
- Prevent regressions by adding more tests


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -588,6 +588,103 @@
   ns::[[Old^Alias]] Bar;
 }
   )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Templated class specialization.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  R"cpp(
+template 
+bool [[F^oo]] = true;
+
+// Explicit template specialization
+template <>
+bool [[F^oo]] = false;
+
+// Partial template specialization
+template 
+bool [[F^oo]] = false;
+
+void foo() {
+  // Ref to the explicit template specialization
+  [[F^oo]];
+  // Ref to the primary template.
+  [[F^oo]];
+}
+  )cpp",
+  R"cpp(
+template 
+void [[f^oo]](T t);
+
+template <>
+void [[f^oo]](int a);
+
+void test() {
+  [[f^oo]](1);
+}
+  )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,12 +18,12 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include 
 
@@ -76,6 +76,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -92,8 +94,7 @@
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
 // Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +223,73 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const NamedDecl *canonicalRenameDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const CXXMethodDecl *D) {
+  const auto *Result = D;
+  if (const auto *InstantiatedMethod = D->getInstantiatedFromMemberFunction())
+Result = cast(InstantiatedMethod);
+  while (Result->isVirtual() && Result->size_overridden_methods())
+Result = *Result->overridden_methods().begin();
+  return Result;
+}
+
+const NamedDecl *canonicalRenameDecl(const FunctionDecl *D) {
+  if (const auto *Template = D->getPrimaryTemplate())
+return canonicalRenameDecl(Template);
+  return D;
+}
+
+const NamedDecl *canonicalRenameDecl(const FunctionTemplateDecl *D) {
+  const auto *TemplatedDecl = D->getTemplatedDecl();
+  if (const auto *Primary = TemplatedDecl->getPrimaryTemplate())
+return Primary;
+  return TemplatedDecl;
+}
+
+const NamedDecl *canonicalRenameDecl(const 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

don't dig into details, first round of comments.

I think the scope is a bit ambiguous, my reading is that it 1) removes old 
clang-rename API usage *and* 2) fixes some existing bugs. I would suggest just 
scoping it on 1).




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:251
+const NamedDecl *canonicalRenameDecl(const FieldDecl *D) {
+  // This is a hacky way to do something like
+  // CXXMethodDecl::getINstantiatedFromMemberFunction for the field because

yeah, this is quite tricky. I'd suggest to defer it to a separate patch. 

The behavior you described in https://github.com/clangd/clangd/issues/582 makes 
sense to me.

Another missing case is about static class members, they are `VarDecl` in the 
AST. 



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:283
+const auto *Definition = RD->getDefinition();
+Candidate = Definition ? Definition : RD->getMostRecentDecl();
+  }

I'm not sure the motivation of the change,  the same as line 244.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:299
+  if (const auto *Field = dyn_cast(Candidate))
+return canonicalRenameDecl(Field);
+  return Candidate;

Looks like we're missing the `VarDecl` case (looks like a regression). I think 
we should probably add tests from 
https://github.com/llvm/llvm-project/commit/1e32df2f91f1aa1f8cd400ce50a621578fa0534e
 to clangd rename test.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:300
+return canonicalRenameDecl(Field);
+  return Candidate;
+}

we're comparing Decl pointers to check whether they point to the same symbol, I 
think we should use `Candidate->getCanonicalDecl()` here.

thinking of a case, in the AST, we have two `Decl*`s, one points to the forward 
decl, the other one points to the definition. but they points to the same 
symbol.

```
class Foo; // forward decl, this is the canonical decl.

class Foo {
};
```




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:307
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and 
its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  const auto *RenameDecl = canonicalRenameDecl();
 

since we call `canonicalRenameDecl` in `locateDeclAt`, I think `ND` is already 
canonical, right?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304729.
kbobyrev added a comment.

Use early return in FieldDecl canonicalization.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -588,6 +588,147 @@
   ns::[[Old^Alias]] Bar;
 }
   )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Templated class specialization.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Fields in classes & partial and full specialiations.
+  R"cpp(
+class Foo {
+public:
+  Foo(int Variable) : [[Variabl^e]](Variable) {}
+
+  int [[Va^riable]] = 42;
+
+private:
+  void foo() { ++[[Vari^able]]; }
+};
+
+void bar() {
+  Foo f(9000);
+  f.[[Variable^]] = -1;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T [[Vari^able]] = 42;
+};
+
+void foo() {
+  Foo f;
+  f.[[Varia^ble]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T Variable;
+  void bar() { ++Variable; }
+};
+
+template<>
+struct Foo {
+  unsigned [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,12 +18,12 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/Error.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/FormatVariadic.h"
 #include 
 
@@ -76,6 +76,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -92,8 +94,7 @@
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
 // Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +223,89 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const NamedDecl 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304500.
kbobyrev added a comment.

I think this is the first version that is ready for a review. Let me know if
you have any questions, @hokein!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -588,6 +588,147 @@
   ns::[[Old^Alias]] Bar;
 }
   )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Templated class specialization.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Fields in classes & partial and full specialiations.
+  R"cpp(
+class Foo {
+public:
+  Foo(int Variable) : [[Variabl^e]](Variable) {}
+
+  int [[Va^riable]] = 42;
+
+private:
+  void foo() { ++[[Vari^able]]; }
+};
+
+void bar() {
+  Foo f(9000);
+  f.[[Variable^]] = -1;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T [[Vari^able]] = 42;
+};
+
+void foo() {
+  Foo f;
+  f.[[Varia^ble]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  T Variable[42];
+  U Another;
+
+  void bar() {}
+};
+
+template
+struct Foo {
+  T Variable;
+  void bar() { ++Variable; }
+};
+
+template<>
+struct Foo {
+  unsigned [[Var^iable]];
+  void bar() { ++[[Var^iable]]; }
+};
+
+void foo() {
+  Foo f;
+  f.[[Var^iable]] = 9000;
+}
+  )cpp",
   };
   llvm::StringRef NewName = "NewName";
   for (llvm::StringRef T : Tests) {
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -18,7 +18,6 @@
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
 #include "clang/Basic/SourceLocation.h"
-#include "clang/Tooling/Refactoring/Rename/USRFindingAction.h"
 #include "clang/Tooling/Syntax/Tokens.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
@@ -76,6 +75,8 @@
   return OtherFile;
 }
 
+const NamedDecl *canonicalRenameDecl(const NamedDecl *D);
+
 llvm::DenseSet locateDeclAt(ParsedAST ,
SourceLocation TokenStartLoc) {
   unsigned Offset =
@@ -92,8 +93,7 @@
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
 // Get to CXXRecordDecl from constructor or destructor.
-D = tooling::getCanonicalSymbolDeclaration(D);
-Result.insert(D);
+Result.insert(canonicalRenameDecl(D));
   }
   return Result;
 }
@@ -222,23 +222,89 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const NamedDecl *canonicalRenameDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const NamedDecl *canonicalRenameDecl(const CXXMethodDecl *D) {
+  const auto *Result = D;
+  if (const auto *InstantiatedMethod = 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Still WIP.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304466.
kbobyrev added a comment.

Rebase on top of master.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -92,6 +92,7 @@
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
 // Get to CXXRecordDecl from constructor or destructor.
+// FIXME(kirillbobyrev): Just use getRenameRoot?
 D = tooling::getCanonicalSymbolDeclaration(D);
 Result.insert(D);
   }
@@ -222,23 +223,80 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const Decl *getRenameRootDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+const Decl *getRenameRootDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const Decl *getRenameRootDecl(const CXXMethodDecl *D) {
+  const auto *Result = D;
+  if (const auto *InstantiatedMethod = D->getInstantiatedFromMemberFunction())
+Result = cast(InstantiatedMethod);
+  while (Result->isVirtual() && Result->size_overridden_methods())
+Result = *Result->overridden_methods().begin();
+  return Result;
+}
+
+const Decl *getRenameRootDecl(const FunctionDecl *D) {
+  const auto *Definition = D->getDefinition();
+  const auto *Candidate = Definition ? Definition : D->getMostRecentDecl();
+  return Candidate->isTemplateInstantiation()
+ ? Candidate->getPrimaryTemplate()->getTemplatedDecl()
+ : Candidate;
+}
+
+const Decl *getRenameRootDecl(const FieldDecl *D) {
+  // This is a hacky way to do something like
+  // CXXMethodDecl::getINstantiatedFromMemberFunction for the field because
+  // Clang AST does not store relevant information about the field that is
+  // instantiated.
+  const auto *TemplateSpec =
+  dyn_cast(D->getParent());
+  if (!TemplateSpec)
+return D;
+  const auto *FieldParent = TemplateSpec->getTemplateInstantiationPattern();
+  if (!FieldParent)
+return D;
+  for (const auto *Field : FieldParent->fields())
+if (Field->getFieldIndex() == D->getFieldIndex() &&
+Field->getLocation() == D->getLocation())
+  return Field;
+  return D;
+}
+
+// FIXME(kirillbobyrev): Write documentation.
+const Decl *getRenameRootDecl(const Decl *D) {
+  const auto *Candidate = D;
+  if (const auto *RD = dyn_cast(D)) {
+const auto *Definition = RD->getDefinition();
+Candidate = Definition ? Definition : RD->getMostRecentDecl();
+  }
+  if (const auto *Template = dyn_cast(Candidate))
+return getRenameRootDecl(Template);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(Candidate))
+return getRenameRootDecl(ClassTemplateSpecialization);
+  if (const auto *Constructor = dyn_cast(Candidate))
+return getRenameRootDecl(Constructor->getParent());
+  if (const auto *Destructor = dyn_cast(Candidate))
+return getRenameRootDecl(Destructor->getParent());
+  if (const auto *Method = dyn_cast(Candidate))
+return getRenameRootDecl(Method);
+  if (const auto *Function = dyn_cast(Candidate))
+return getRenameRootDecl(Function);
+  if (const auto *Field = dyn_cast(Candidate))
+return getRenameRootDecl(Field);
+  return Candidate;
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrencesWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  const auto *RenameDeclRoot = getRenameRootDecl();
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
@@ -246,11 +304,11 @@
   if (Ref.Targets.empty())
 return;
   for (const auto *Target : Ref.Targets) {
-auto ID = getSymbolID(Target);
-if (!ID || TargetIDs.find(ID) == TargetIDs.end())
+if (getRenameRootDecl(Target) == RenameDeclRoot) {
+  Results.push_back(Ref.NameLoc);
   return;
+}
   }
-  

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304464.
kbobyrev added a comment.

Actually reset RenameTests.cpp


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp

Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -92,6 +92,7 @@
targetDecl(SelectedNode->ASTNode,
   DeclRelation::Alias | DeclRelation::TemplatePattern)) {
 // Get to CXXRecordDecl from constructor or destructor.
+// FIXME(kirillbobyrev): Just use getRenameRoot?
 D = tooling::getCanonicalSymbolDeclaration(D);
 Result.insert(D);
   }
@@ -219,23 +220,80 @@
   return error("Cannot rename symbol: {0}", Message(Reason));
 }
 
+const Decl *getRenameRootDecl(const ClassTemplateSpecializationDecl *D) {
+  return D->getSpecializedTemplate()->getTemplatedDecl();
+}
+
+const Decl *getRenameRootDecl(const TemplateDecl *D) {
+  return D->getTemplatedDecl();
+}
+
+const Decl *getRenameRootDecl(const CXXMethodDecl *D) {
+  const auto *Result = D;
+  if (const auto *InstantiatedMethod = D->getInstantiatedFromMemberFunction())
+Result = cast(InstantiatedMethod);
+  while (Result->isVirtual() && Result->size_overridden_methods())
+Result = *Result->overridden_methods().begin();
+  return Result;
+}
+
+const Decl *getRenameRootDecl(const FunctionDecl *D) {
+  const auto *Definition = D->getDefinition();
+  const auto *Candidate = Definition ? Definition : D->getMostRecentDecl();
+  return Candidate->isTemplateInstantiation()
+ ? Candidate->getPrimaryTemplate()->getTemplatedDecl()
+ : Candidate;
+}
+
+const Decl *getRenameRootDecl(const FieldDecl *D) {
+  // This is a hacky way to do something like
+  // CXXMethodDecl::getINstantiatedFromMemberFunction for the field because
+  // Clang AST does not store relevant information about the field that is
+  // instantiated.
+  const auto *TemplateSpec =
+  dyn_cast(D->getParent());
+  if (!TemplateSpec)
+return D;
+  const auto *FieldParent = TemplateSpec->getTemplateInstantiationPattern();
+  if (!FieldParent)
+return D;
+  for (const auto *Field : FieldParent->fields())
+if (Field->getFieldIndex() == D->getFieldIndex() &&
+Field->getLocation() == D->getLocation())
+  return Field;
+  return D;
+}
+
+// FIXME(kirillbobyrev): Write documentation.
+const Decl *getRenameRootDecl(const Decl *D) {
+  const auto *Candidate = D;
+  if (const auto *RD = dyn_cast(D)) {
+const auto *Definition = RD->getDefinition();
+Candidate = Definition ? Definition : RD->getMostRecentDecl();
+  }
+  if (const auto *Template = dyn_cast(Candidate))
+return getRenameRootDecl(Template);
+  if (const auto *ClassTemplateSpecialization =
+  dyn_cast(Candidate))
+return getRenameRootDecl(ClassTemplateSpecialization);
+  if (const auto *Constructor = dyn_cast(Candidate))
+return getRenameRootDecl(Constructor->getParent());
+  if (const auto *Destructor = dyn_cast(Candidate))
+return getRenameRootDecl(Destructor->getParent());
+  if (const auto *Method = dyn_cast(Candidate))
+return getRenameRootDecl(Method);
+  if (const auto *Function = dyn_cast(Candidate))
+return getRenameRootDecl(Function);
+  if (const auto *Field = dyn_cast(Candidate))
+return getRenameRootDecl(Field);
+  return Candidate;
+}
+
 // Return all rename occurrences in the main file.
 std::vector findOccurrencesWithinFile(ParsedAST ,
   const NamedDecl ) {
   trace::Span Tracer("FindOccurrenceeWithinFile");
-  // If the cursor is at the underlying CXXRecordDecl of the
-  // ClassTemplateDecl, ND will be the CXXRecordDecl. In this case, we need to
-  // get the primary template manually.
-  // getUSRsForDeclaration will find other related symbols, e.g. virtual and its
-  // overriddens, primary template and all explicit specializations.
-  // FIXME: Get rid of the remaining tooling APIs.
-  const auto *RenameDecl =
-  ND.getDescribedTemplate() ? ND.getDescribedTemplate() : 
-  std::vector RenameUSRs =
-  tooling::getUSRsForDeclaration(RenameDecl, AST.getASTContext());
-  llvm::DenseSet TargetIDs;
-  for (auto  : RenameUSRs)
-TargetIDs.insert(SymbolID(USR));
+  const auto *RenameDeclRoot = getRenameRootDecl();
 
   std::vector Results;
   for (Decl *TopLevelDecl : AST.getLocalTopLevelDecls()) {
@@ -243,11 +301,11 @@
   if (Ref.Targets.empty())
 return;
   for (const auto *Target : Ref.Targets) {
-auto ID = getSymbolID(Target);
-if (!ID || TargetIDs.find(ID) == TargetIDs.end())
+if (getRenameRootDecl(Target) == RenameDeclRoot) {
+  Results.push_back(Ref.NameLoc);
   return;
+}
   }
-  

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 304463.
kbobyrev added a comment.

Reset unittests to HEAD.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -91,10 +91,86 @@
 }
 
 TEST(RenameTest, WithinFileRename) {
-  // rename is runnning on all "^" points, and "[[]]" ranges point to the
-  // identifier that is being renamed.
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
   llvm::StringRef Tests[] = {
-  // Function.
+  // Templated static method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Class template (partial) specialization forward declarations.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+
+  // Class template (full) specialization forward declaration.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Function template specialization forward declaration without function
+  // definition.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Simple recursive function.
   R"cpp(
 void [[foo^]]() {
   [[fo^o]]();
@@ -104,7 +180,7 @@
   // Type.
   R"cpp(
 struct [[foo^]] {};
-[[foo]] test() {
+[[foo^]] test() {
[[f^oo]] x;
return x;
 }
@@ -114,20 +190,21 @@
   R"cpp(
 void bar() {
   if (auto [[^foo]] = 5) {
-[[foo]] = 3;
+[[fo^o]] = 3;
   }
 }
   )cpp",
 
-  // Rename class, including constructor/destructor.
+  // Class, its constructor and destructor.
   R"cpp(
 class [[F^oo]] {
+public:
   [[F^oo]]();
-  ~[[Foo]]();
+  ~[[Fo^o]]();
   void foo(int x);
 };
-[[Foo]]::[[Fo^o]]() {}
-void [[Foo]]::foo(int x) {}
+[[Fo^o]]::[[Fo^o]]() {}
+void [[Fo^o]]::foo(int x) {}
   )cpp",
 
   // Rename template class, including constructor/destructor.
@@ -199,9 +276,9 @@
 class [[F^oo]] {};
 
 void test() {
-  [[Foo]] x;
-  [[Foo]] y;
-  [[Foo]] z;
+  [[F^oo]] x;
+  [[Fo^o]] y;
+  [[Foo^]] z;
 }
   )cpp",
 
@@ -360,8 +437,8 @@
 void boo(int);
 
 void qoo() {
-  [[foo]]();
-  boo([[foo]]());
+  [[f^oo]]();
+  boo([[fo^o]]());
   M1();
   boo(M1());
   M2([[foo]]());
@@ -454,7 +531,7 @@
 }
   )cpp",
 
-  // template class in template argument list.
+  // Template class in template argument list.
   R"cpp(
 template
 class [[Fo^o]] {};
@@ -510,6 +587,263 @@
   }
 }
 
+TEST(RenameTest, Alias) {
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
+  llvm::StringRef Tests[] = {
+  R"cpp(
+class X {};
+typedef X [[Fo^o]];
+  )cpp",
+
+  R"cpp(
+class X {};
+using [[U^Old]] = X;
+  )cpp",
+
+  R"cpp(
+template 
+class X { T t; };
+
+template 
+using [[O^ld]] = X;
+  )cpp",
+
+  R"cpp(
+namespace x { class X {}; }
+namespace ns {
+using [[F^oo]] = x::X;
+}
+  )cpp",
+
+  R"cpp(
+namespace x { class Old {}; }
+namespace ns {
+#define REF(alias) alias alias_var;
+
+#define ALIAS(old) \
+  using old##Alias = x::old; \

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

Still WIP, moving test changes to a separate chain of patches (starts with 
D91102 ) to make review easier and start the 
pipeline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 303940.
kbobyrev added a comment.

Finish fields canonicalization.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -91,10 +91,86 @@
 }
 
 TEST(RenameTest, WithinFileRename) {
-  // rename is runnning on all "^" points, and "[[]]" ranges point to the
-  // identifier that is being renamed.
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
   llvm::StringRef Tests[] = {
-  // Function.
+  // Templated static method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Class template (partial) specialization forward declarations.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+
+  // Class template (full) specialization forward declaration.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Function template specialization forward declaration without function
+  // definition.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Simple recursive function.
   R"cpp(
 void [[foo^]]() {
   [[fo^o]]();
@@ -104,7 +180,7 @@
   // Type.
   R"cpp(
 struct [[foo^]] {};
-[[foo]] test() {
+[[foo^]] test() {
[[f^oo]] x;
return x;
 }
@@ -114,20 +190,21 @@
   R"cpp(
 void bar() {
   if (auto [[^foo]] = 5) {
-[[foo]] = 3;
+[[fo^o]] = 3;
   }
 }
   )cpp",
 
-  // Rename class, including constructor/destructor.
+  // Class, its constructor and destructor.
   R"cpp(
 class [[F^oo]] {
+public:
   [[F^oo]]();
-  ~[[Foo]]();
+  ~[[Fo^o]]();
   void foo(int x);
 };
-[[Foo]]::[[Fo^o]]() {}
-void [[Foo]]::foo(int x) {}
+[[Fo^o]]::[[Fo^o]]() {}
+void [[Fo^o]]::foo(int x) {}
   )cpp",
 
   // Rename template class, including constructor/destructor.
@@ -199,9 +276,9 @@
 class [[F^oo]] {};
 
 void test() {
-  [[Foo]] x;
-  [[Foo]] y;
-  [[Foo]] z;
+  [[F^oo]] x;
+  [[Fo^o]] y;
+  [[Foo^]] z;
 }
   )cpp",
 
@@ -360,8 +437,8 @@
 void boo(int);
 
 void qoo() {
-  [[foo]]();
-  boo([[foo]]());
+  [[f^oo]]();
+  boo([[fo^o]]());
   M1();
   boo(M1());
   M2([[foo]]());
@@ -454,7 +531,7 @@
 }
   )cpp",
 
-  // template class in template argument list.
+  // Template class in template argument list.
   R"cpp(
 template
 class [[Fo^o]] {};
@@ -510,6 +587,263 @@
   }
 }
 
+TEST(RenameTest, Alias) {
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
+  llvm::StringRef Tests[] = {
+  R"cpp(
+class X {};
+typedef X [[Fo^o]];
+  )cpp",
+
+  R"cpp(
+class X {};
+using [[U^Old]] = X;
+  )cpp",
+
+  R"cpp(
+template 
+class X { T t; };
+
+template 
+using [[O^ld]] = X;
+  )cpp",
+
+  R"cpp(
+namespace x { class X {}; }
+namespace ns {
+using [[F^oo]] = x::X;
+}
+  )cpp",
+
+  R"cpp(
+namespace x { class Old {}; }
+namespace ns {
+#define REF(alias) alias alias_var;
+
+#define ALIAS(old) \
+  using old##Alias = 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 303733.
kbobyrev added a comment.

Store progress.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -91,10 +91,86 @@
 }
 
 TEST(RenameTest, WithinFileRename) {
-  // rename is runnning on all "^" points, and "[[]]" ranges point to the
-  // identifier that is being renamed.
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
   llvm::StringRef Tests[] = {
-  // Function.
+  // Templated static method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Class template (partial) specialization forward declarations.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+
+  // Class template (full) specialization forward declaration.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Function template specialization forward declaration without function
+  // definition.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Simple recursive function.
   R"cpp(
 void [[foo^]]() {
   [[fo^o]]();
@@ -104,7 +180,7 @@
   // Type.
   R"cpp(
 struct [[foo^]] {};
-[[foo]] test() {
+[[foo^]] test() {
[[f^oo]] x;
return x;
 }
@@ -114,20 +190,21 @@
   R"cpp(
 void bar() {
   if (auto [[^foo]] = 5) {
-[[foo]] = 3;
+[[fo^o]] = 3;
   }
 }
   )cpp",
 
-  // Rename class, including constructor/destructor.
+  // Class, its constructor and destructor.
   R"cpp(
 class [[F^oo]] {
+public:
   [[F^oo]]();
-  ~[[Foo]]();
+  ~[[Fo^o]]();
   void foo(int x);
 };
-[[Foo]]::[[Fo^o]]() {}
-void [[Foo]]::foo(int x) {}
+[[Fo^o]]::[[Fo^o]]() {}
+void [[Fo^o]]::foo(int x) {}
   )cpp",
 
   // Rename template class, including constructor/destructor.
@@ -199,9 +276,9 @@
 class [[F^oo]] {};
 
 void test() {
-  [[Foo]] x;
-  [[Foo]] y;
-  [[Foo]] z;
+  [[F^oo]] x;
+  [[Fo^o]] y;
+  [[Foo^]] z;
 }
   )cpp",
 
@@ -360,8 +437,8 @@
 void boo(int);
 
 void qoo() {
-  [[foo]]();
-  boo([[foo]]());
+  [[f^oo]]();
+  boo([[fo^o]]());
   M1();
   boo(M1());
   M2([[foo]]());
@@ -454,7 +531,7 @@
 }
   )cpp",
 
-  // template class in template argument list.
+  // Template class in template argument list.
   R"cpp(
 template
 class [[Fo^o]] {};
@@ -510,6 +587,206 @@
   }
 }
 
+TEST(RenameTest, Alias) {
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
+  llvm::StringRef Tests[] = {
+  R"cpp(
+class X {};
+typedef X [[Fo^o]];
+  )cpp",
+
+  R"cpp(
+class X {};
+using [[U^Old]] = X;
+  )cpp",
+
+  R"cpp(
+template 
+class X { T t; };
+
+template 
+using [[O^ld]] = X;
+  )cpp",
+
+  R"cpp(
+namespace x { class X {}; }
+namespace ns {
+using [[F^oo]] = x::X;
+}
+  )cpp",
+
+  R"cpp(
+namespace x { class Old {}; }
+namespace ns {
+#define REF(alias) alias alias_var;
+
+#define ALIAS(old) \
+  using old##Alias = x::old; \
+ 

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 303397.
kbobyrev added a comment.

Continue implementation, integrate more tests from clang-rename.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -91,10 +91,86 @@
 }
 
 TEST(RenameTest, WithinFileRename) {
-  // rename is runnning on all "^" points, and "[[]]" ranges point to the
-  // identifier that is being renamed.
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
   llvm::StringRef Tests[] = {
-  // Function.
+  // Templated static method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+};
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Class template (partial) specialization forward declarations.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+
+  // Class template (full) specialization forward declaration.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Function template specialization forward declaration without function
+  // definition.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Simple recursive function.
   R"cpp(
 void [[foo^]]() {
   [[fo^o]]();
@@ -104,7 +180,7 @@
   // Type.
   R"cpp(
 struct [[foo^]] {};
-[[foo]] test() {
+[[foo^]] test() {
[[f^oo]] x;
return x;
 }
@@ -114,20 +190,21 @@
   R"cpp(
 void bar() {
   if (auto [[^foo]] = 5) {
-[[foo]] = 3;
+[[fo^o]] = 3;
   }
 }
   )cpp",
 
-  // Rename class, including constructor/destructor.
+  // Class, its constructor and destructor.
   R"cpp(
 class [[F^oo]] {
+public:
   [[F^oo]]();
-  ~[[Foo]]();
+  ~[[Fo^o]]();
   void foo(int x);
 };
-[[Foo]]::[[Fo^o]]() {}
-void [[Foo]]::foo(int x) {}
+[[Fo^o]]::[[Fo^o]]() {}
+void [[Fo^o]]::foo(int x) {}
   )cpp",
 
   // Rename template class, including constructor/destructor.
@@ -199,9 +276,9 @@
 class [[F^oo]] {};
 
 void test() {
-  [[Foo]] x;
-  [[Foo]] y;
-  [[Foo]] z;
+  [[F^oo]] x;
+  [[Fo^o]] y;
+  [[Foo^]] z;
 }
   )cpp",
 
@@ -361,7 +438,7 @@
 
 void qoo() {
   [[foo]]();
-  boo([[foo]]());
+  boo([[fo^o]]());
   M1();
   boo(M1());
   M2([[foo]]());
@@ -454,7 +531,7 @@
 }
   )cpp",
 
-  // template class in template argument list.
+  // Template class in template argument list.
   R"cpp(
 template
 class [[Fo^o]] {};
@@ -510,6 +587,151 @@
   }
 }
 
+TEST(RenameTest, Alias) {
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
+  llvm::StringRef Tests[] = {
+  R"cpp(
+class X {};
+typedef X [[Fo^o]];
+  )cpp",
+
+  R"cpp(
+class X {};
+using [[U^Old]] = X;
+  )cpp",
+
+  R"cpp(
+template 
+class X { T t; };
+
+template 
+using [[O^ld]] = X;
+  )cpp",
+
+  R"cpp(
+namespace x { class X {}; }
+namespace ns {
+using [[F^oo]] = x::X;
+}
+  )cpp",
+
+  R"cpp(
+namespace x { class Old {}; }
+namespace ns {
+#define REF(alias) alias alias_var;
+
+#define ALIAS(old) \
+  using old##Alias = x::old; \
+   

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61107 tests passed, 0 failed 
and 728 were skipped.

{icon times-circle color=red} clang-tidy: fail. Please fix clang-tidy findings 
.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880



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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev planned changes to this revision.
kbobyrev added a comment.

This is a WIP draft. Few items to address before pushing for a review

- More tests: renaming is a complex feature and I want to make sure there are 
no regressions/corner-cases I did not handle. It might make sense to adopt a 
bunch of tests from Clang-Rename and also add a number of new tests.
- Detailed documentation: explanation of AST navigation used here, description 
of limitations (virtual functions "incorrect" handling, notes on performance, 
etc).
- Better naming

The logical change after this patch might be changing Clang-Rename to rely on 
this API and completely removing USR-based approach, because it is no longer 
maintained and is unlikely to evolve to match the expectations of existing 
users, but this might require some substantial infrastructure changes and I'm 
not planning to do that soon.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71880/new/

https://reviews.llvm.org/D71880



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

[PATCH] D71880: [clangd] Implement Decl canonicalization rules for rename

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.

This patch introduces new canonicalization rules which are used for AST-based
rename in Clangd. By comparing two canonical declarations of inspected nodes,
Clangd determines whether both of them belong to the same entity user would
like to rename. Such functionality is relatively concise compared to the
Clang-Rename API that is used right now. It also helps to overcome the
limitations that Clang-Rename originally had and helps to eliminate several
classes of bugs.

Clangd AST-based rename currently relies on Clangd-Rename functionality, which
has a better test coverage and hence provides some correctness guarantees. This
pattch decouples two rename features and therefore introduces new challenges to
the Clangd maintenance. To prevent regressions and improve stability of Clangd
rename feature, there are new unittests that come with this change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71880

Files:
  clang-tools-extra/clangd/refactor/Rename.cpp
  clang-tools-extra/clangd/unittests/RenameTests.cpp

Index: clang-tools-extra/clangd/unittests/RenameTests.cpp
===
--- clang-tools-extra/clangd/unittests/RenameTests.cpp
+++ clang-tools-extra/clangd/unittests/RenameTests.cpp
@@ -88,10 +88,87 @@
 }
 
 TEST(RenameTest, WithinFileRename) {
-  // rename is runnning on all "^" points, and "[[]]" ranges point to the
-  // identifier that is being renamed.
+  // For each "^" this test moves cursor to its location and applies renaming
+  // while checking that all identifiers enclosed in [[]] ranges are handled
+  // correctly.
+  // TODO(kirillbobyrev): Add more tests.
   llvm::StringRef Tests[] = {
-  // Function.
+  // Templated static method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  static T [[f^oo]]() {}
+}
+
+void bar() {
+  Foo::[[f^oo]]();
+}
+  )cpp",
+
+  // Templated method instantiation.
+  R"cpp(
+template
+class Foo {
+public:
+  T [[f^oo]]() {}
+}
+
+void bar() {
+  Foo().[[f^oo]]();
+}
+  )cpp",
+
+  // Class template (partial) specialization forward declarations.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+
+template
+class [[Foo^]];
+  )cpp",
+
+  // Class template (full) specialization forward declaration.
+  R"cpp(
+template
+class [[Foo^]];
+
+template
+class [[Foo^]] {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]() {};
+  )cpp",
+
+  // Function template specialization forward declaration.
+  R"cpp(
+template
+U [[foo^]]() {};
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Function template specialization forward declaration without function
+  // definition.
+  R"cpp(
+template
+U [[foo^]]();
+
+template
+U [[foo^]]();
+  )cpp",
+
+  // Simple recursive function.
   R"cpp(
 void [[foo^]]() {
   [[fo^o]]();
@@ -116,15 +193,16 @@
 }
   )cpp",
 
-  // Rename class, including constructor/destructor.
+  // Class, its constructor and destructor.
   R"cpp(
 class [[F^oo]] {
+public:
   [[F^oo]]();
-  ~[[Foo]]();
+  ~[[Fo^o]]();
   void foo(int x);
 };
-[[Foo]]::[[Fo^o]]() {}
-void [[Foo]]::foo(int x) {}
+[[Fo^o]]::[[Fo^o]]() {}
+void [[Fo^o]]::foo(int x) {}
   )cpp",
 
   // Class in template argument.
@@ -175,9 +253,9 @@
 class [[F^oo]] {};
 
 void test() {
-  [[Foo]] x;
-  [[Foo]] y;
-  [[Foo]] z;
+  [[F^oo]] x;
+  [[Fo^o]] y;
+  [[Foo^]] z;
 }
   )cpp",
 
@@ -303,7 +381,7 @@
 
 void qoo() {
   [[foo]]();
-  boo([[foo]]());
+  boo([[fo^o]]());
   M1();
   boo(M1());
   M2([[foo]]());
@@ -396,7 +474,7 @@
 }
   )cpp",
 
-  // template class in template argument list.
+  // Template class in template argument list.
   R"cpp(
 template
 class [[Fo^o]] {};
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -209,6 +209,57 @@
   llvm::inconvertibleErrorCode());
 }
 
+const Decl *getRenameRootDecl(const ClassTemplateSpecializationDecl *D) {
+  return