[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