[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<CXXRecordDecl>(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<typename T>
----------------
can you add a template partial testcase? something like 

```
template <typename T, typename U> struct Foo { static T Variable; };

template <typename T> struct Foo<T, bool> {
  static T Variable;
};

void test() {
  Foo<int, bool>::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