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
