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