[PATCH] D132797: [clangd] Support renaming virtual methods

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG

IIRC Tom doesn't have commit access, so I'll apply the last trivial changes and commit to avoid another round trip.
(Please LMK if i'm wrong about this!)



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:561-562
+    Index.relations(Req, [&](const SymbolID &, const Symbol &Override) {
+      IDs.insert(Override.ID);
+      Pending.insert(Override.ID);
+    });
----------------
ilya-biryukov wrote:
> This should protect against infinite loop in case of broken/stale index.
Good catch!


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:245
   trace::Span Tracer("FindOccurrencesWithinFile");
   assert(canonicalRenameDecl(&ND) == &ND &&
          "ND should be already canonicalized.");
----------------
tom-anders wrote:
> sammccall wrote:
> > Unfortunately we've uncovered a bug: this assertion fires (after this patch) on the following code:
> > 
> > ```
> > template <typename> class Foo { virtual void m(); };
> > class Bar : Foo<int> { void ^m() override; };
> > ```
> > 
> > `canonicalRenameDecl` is supposed to be idempotent. However:`canonicalRenameDecl(Bar::m)` is `Foo<int>::m`, but `canonicalRenameDecl(Foo<int>::m)` is `Foo<T>::m`.
> > 
> > I think this is because the `getInstantiatedFromMemberFunction` check comes before the `overridden_methods` check, so if the override points to a member of an instantiation then it's too late to map it back to the member of the template.
> > 
> > Probably the best fix is to change line 103 to a recursive `return canonicalRenameDecl(...)`, as is done elsewhere in the function.
> > 
> > (Can you please add this case to the tests?)
> I added the test case for this. However, your suggested fix doesn't seem to stop the assertion from being triggered. I'd be grateful if you can take a closer look at this in a separate review - Should I leave the test case in anyway? Is it okay if the test fails for a while until you found the fix?
Thanks for adding the test, and sorry about the bad guess at a fix. We can't turn the buildbots red, so I'll comment out the test and fix+enable it separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132797