[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGabfcb606c2f8: [clangd] Add support for within-file rename of 
complicated fields (authored by kbobyrev).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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
@@ -540,6 +540,94 @@
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  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",
+  // Static fields.
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  static T [[Var^iable]];
+};
+
+template <>
+int Foo::[[Var^iable]] = 42;
+
+template <>
+bool Foo::[[Var^iable]] = true;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+  bool LocalBool = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -124,6 +124,28 @@
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(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 *FieldParent = dyn_cast(Field->getParent());
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getDeclName() == Candidate->getDeclName())
+return Candidate->getCanonicalDecl();
+elog("FieldParent should have field with the same name as Field.");
+  }
+  if (const auto *VD = dyn_cast(D)) {
+if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
+  VD = OriginalVD;
+return VD->getCanonicalDecl();
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307834.
kbobyrev marked 3 inline comments as done.
kbobyrev added a comment.

Resolve another round of comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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
@@ -540,6 +540,94 @@
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  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",
+  // Static fields.
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  static T [[Var^iable]];
+};
+
+template <>
+int Foo::[[Var^iable]] = 42;
+
+template <>
+bool Foo::[[Var^iable]] = true;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+  bool LocalBool = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -124,6 +124,28 @@
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(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 *FieldParent = dyn_cast(Field->getParent());
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getDeclName() == Candidate->getDeclName())
+return Candidate->getCanonicalDecl();
+elog("FieldParent should have field with the same name as Field.");
+  }
+  if (const auto *VD = dyn_cast(D)) {
+if (const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember())
+  VD = OriginalVD;
+return VD->getCanonicalDecl();
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:132
+// instantiated.
+// FIXME: Maybe a proper way of fixing this would be enhancing Clang AST
+// although it might be a bigger effort.

not sure the `FIXME` is useful here, we may never do that, I'd just drop it.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:138
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+if (!FieldParent)
+  return Field->getCanonicalDecl();

nit: this if branch is not necessary, because the if below already handles that.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:150
+const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember();
+if (OriginalVD)
+  VD = OriginalVD;

nit: we can inline the OriginalVD declaration into the `if`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev updated this revision to Diff 307818.
kbobyrev marked 6 inline comments as done.
kbobyrev added a comment.

Resolve remaining comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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
@@ -540,6 +540,94 @@
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  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",
+  // Static fields.
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  static T [[Var^iable]];
+};
+
+template <>
+int Foo::[[Var^iable]] = 42;
+
+template <>
+bool Foo::[[Var^iable]] = true;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+  bool LocalBool = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -124,6 +124,33 @@
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(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.
+// FIXME: Maybe a proper way of fixing this would be enhancing Clang AST
+// although it might be a bigger effort.
+const auto *FieldParent = dyn_cast(Field->getParent());
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+FieldParent = FieldParent->getTemplateInstantiationPattern();
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getDeclName() == Candidate->getDeclName())
+return Candidate->getCanonicalDecl();
+elog("FieldParent should have field with the same name as Field.");
+  }
+  if (const auto *VD = dyn_cast(D)) {
+const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember();
+if (OriginalVD)
+  VD = OriginalVD;
+return VD->getCanonicalDecl();
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:134
+// although it might be a bigger effort.
+const auto *FieldParent = dyn_cast(Field->getParent())
+  ->getTemplateInstantiationPattern();

sorry, I thought `Field->getParent()` returns a `CXXRecordDecl`. 

I think this is dangerous for C, getParent returns a `RecordDecl`, then 
dyn_cast returns null, and we will access a nullptr.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:142
+for (const FieldDecl *Candidate : FieldParent->fields()) {
+  if (Field->getFieldIndex() == Candidate->getFieldIndex()) {
+assert(Field->getLocation() == Candidate->getLocation() &&

kbobyrev wrote:
> hokein wrote:
> > I think we could also check the equality of their names.
> Yes, that is what @sammccall proposed too, but names not simple `unsigned` 
> numbers, checking for that is just more expensive.
I think I meant to use `Field->getDeclName() == Candidate->getDeclName()`, 
`DeclarationName` comparison is cheap, just comparing with the underlying 
pointer -- clang will unique all identifiers in the  `IdentifierTable`.



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:613
+  )cpp",
+  R"cpp(
+template

can you add a template partial testcase? something like 

```
template  struct Foo { static T Variable; };

template  struct Foo {
  static T Variable;
};

void test() {
  Foo::Variable = 5;
}
```



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:575
+  T Variable[42];
+  U Another;
+

kbobyrev wrote:
> hokein wrote:
> > `Another` seems to be not needed. I'd suggest removing it to make the 
> > testcase as tense as possible.
> In this case there is only one field and I'd be happy to check that the 
> correct field gets renamed in case there are two of them (this is when the 
> index comparison is checked). Otherwise this would pass without the index 
> checking (there is one field, we don't check if it's actually the one we need 
> and it gets renamed regardless).
I see, for index comparison. this makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

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

Added support for static templated fields. Ready for the review again.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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
@@ -540,6 +540,94 @@
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  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",
+  // Static fields.
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+  R"cpp(
+template
+struct Foo {
+  static T [[Var^iable]];
+};
+
+template <>
+int Foo::[[Var^iable]] = 42;
+
+template <>
+bool Foo::[[Var^iable]] = true;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+  bool LocalBool = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -124,6 +124,31 @@
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(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.
+// FIXME: Maybe a proper way of fixing this would be enhancing Clang AST
+// although it might be a bigger effort.
+const auto *FieldParent = dyn_cast(Field->getParent())
+  ->getTemplateInstantiationPattern();
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getFieldIndex() == Candidate->getFieldIndex())
+return Candidate->getCanonicalDecl();
+elog("FieldParent should have field with same index as Field.");
+  }
+  if (const auto *VD = dyn_cast(D)) {
+const VarDecl *OriginalVD = VD->getInstantiatedFromStaticDataMember();
+if (OriginalVD)
+  VD = OriginalVD;
+return VD->getCanonicalDecl();
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

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

  template
  struct Foo {
static T [[Var^iable]];
  };
  
  template <>
  int Foo::[[Variable^]] = 42;
  
  template <>
  bool Foo::[[Variable^]] = true;
  
  void foo() {
int LocalInt = Foo::[[Variable^]];
bool LocalBool = Foo::[[Variable^]];
  }

This doesn't work yet, need to investigate.




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:131
+// Clang AST does not store relevant information about the field that is
+// instantiated.
+const auto *TemplateSpec =

hokein wrote:
> yeah, this is a bit unfortunate, I don't have a better solution (extending 
> the AST is one option, but it may be not easy). I think we can live with the 
> heuristic.
Yes, I thought about that but realized it might be too much effort, maybe I 
should put a FIXME here.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:142
+for (const FieldDecl *Candidate : FieldParent->fields()) {
+  if (Field->getFieldIndex() == Candidate->getFieldIndex()) {
+assert(Field->getLocation() == Candidate->getLocation() &&

hokein wrote:
> I think we could also check the equality of their names.
Yes, that is what @sammccall proposed too, but names not simple `unsigned` 
numbers, checking for that is just more expensive.



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:575
+  T Variable[42];
+  U Another;
+

hokein wrote:
> `Another` seems to be not needed. I'd suggest removing it to make the 
> testcase as tense as possible.
In this case there is only one field and I'd be happy to check that the correct 
field gets renamed in case there are two of them (this is when the index 
comparison is checked). Otherwise this would pass without the index checking 
(there is one field, we don't check if it's actually the one we need and it 
gets renamed regardless).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

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

Resolve most actionable comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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
@@ -540,6 +540,76 @@
 }
   )cpp",
 
+  // Fields in classes & partial and full specialiations.
+  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",
+  R"cpp(
+struct Foo {
+  static int [[Var^iable]];
+};
+
+int Foo::[[Var^iable]] = 42;
+
+void foo() {
+  int LocalInt = Foo::[[Var^iable]];
+}
+  )cpp",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -119,11 +119,28 @@
 // declaration.
 while (Method->isVirtual() && Method->size_overridden_methods())
   Method = *Method->overridden_methods().begin();
-return dyn_cast(Method->getCanonicalDecl());
+return Method->getCanonicalDecl();
   }
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(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 *FieldParent = dyn_cast(Field->getParent())
+  ->getTemplateInstantiationPattern();
+if (!FieldParent)
+  return Field->getCanonicalDecl();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields())
+  if (Field->getFieldIndex() == Candidate->getFieldIndex())
+return Candidate->getCanonicalDecl();
+llvm_unreachable("FieldParent should have field with same index as Field.");
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

I assume this fixes https://github.com/clangd/clangd/issues/582?

can you check the static members in template classes etc? I think the AST is 
different.




Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:131
+// Clang AST does not store relevant information about the field that is
+// instantiated.
+const auto *TemplateSpec =

yeah, this is a bit unfortunate, I don't have a better solution (extending the 
AST is one option, but it may be not easy). I think we can live with the 
heuristic.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:137
+const CXXRecordDecl *FieldParent =
+TemplateSpec->getTemplateInstantiationPattern();
+// Field is not instantiation.

nit: I think we can simplify the code a bit more:

```
const auto* Template = Field->getParent()->getTemplateInstantiationPattern();
if (!Template)
  return...;

```



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:142
+for (const FieldDecl *Candidate : FieldParent->fields()) {
+  if (Field->getFieldIndex() == Candidate->getFieldIndex()) {
+assert(Field->getLocation() == Candidate->getLocation() &&

I think we could also check the equality of their names.



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:143
+  if (Field->getFieldIndex() == Candidate->getFieldIndex()) {
+assert(Field->getLocation() == Candidate->getLocation() &&
+   "Field should be generated from Candidate so it has the same "

this assertion seems too strong to me -- this is a heuristics, it may have 
false positives. I think we can remove it. 



Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:149
+}
+llvm_unreachable("FieldParent should have field with same index as 
Field.");
+  }

use `elog`?



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:545
+  R"cpp(
+class Foo {
+public:

I think this is a normal rename-field case, it should already work prior to the 
patch.



Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:575
+  T Variable[42];
+  U Another;
+

`Another` seems to be not needed. I'd suggest removing it to make the testcase 
as tense as possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91952

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

[PATCH] D91952: [clangd] Add support for within-file rename of complicated fields

[Kirill Bobyrev via Phabricator via cfe-commits]

kbobyrev created this revision.
kbobyrev added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, arphaman.
Herald added a project: clang.
kbobyrev requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.

This was originally a part of D71880  but is 
separated for simplicity and ease
of reviewing.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D91952

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
@@ -540,6 +540,81 @@
 }
   )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",
+
   // Template parameters.
   R"cpp(
 template 
Index: clang-tools-extra/clangd/refactor/Rename.cpp
===
--- clang-tools-extra/clangd/refactor/Rename.cpp
+++ clang-tools-extra/clangd/refactor/Rename.cpp
@@ -119,11 +119,35 @@
 // declaration.
 while (Method->isVirtual() && Method->size_overridden_methods())
   Method = *Method->overridden_methods().begin();
-return dyn_cast(Method->getCanonicalDecl());
+return Method->getCanonicalDecl();
   }
   if (const auto *Function = dyn_cast(D))
 if (const FunctionTemplateDecl *Template = Function->getPrimaryTemplate())
   return canonicalRenameDecl(Template);
+  if (const auto *Field = dyn_cast(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(Field->getParent());
+if (!TemplateSpec)
+  return Field->getCanonicalDecl();
+const CXXRecordDecl *FieldParent =
+TemplateSpec->getTemplateInstantiationPattern();
+// Field is not instantiation.
+if (!FieldParent || Field->getParent() == FieldParent)
+  return Field->getCanonicalDecl();
+for (const FieldDecl *Candidate : FieldParent->fields()) {
+  if (Field->getFieldIndex() == Candidate->getFieldIndex()) {
+assert(Field->getLocation() == Candidate->getLocation() &&
+   "Field should be generated from Candidate so it has the same "
+   "location.");
+return Candidate->getCanonicalDecl();
+  }
+}
+llvm_unreachable("FieldParent should have field with same index as Field.");
+  }
   return dyn_cast(D->getCanonicalDecl());
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits