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

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry for the delay, this patch is really neat!

The only serious thing is it uncovers an existing bug which asserts. As a potential new crash I think we should fix that - LMK whether you feel comfortable adding that fix in here or you'd like me to do it as a separate review.



================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:245
   trace::Span Tracer("FindOccurrencesWithinFile");
   assert(canonicalRenameDecl(&ND) == &ND &&
          "ND should be already canonicalized.");
----------------
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?)


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:547
 
+namespace {
+void recursivelyInsertOverrides(const SymbolID &Base,
----------------
we're already in an anon namespace here


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:548
+namespace {
+void recursivelyInsertOverrides(const SymbolID &Base,
+                                llvm::DenseSet<SymbolID> &IDs,
----------------
The fact that we only need to walk down wasn't obvious to me at first, maybe add a comment on this function? e.g.

```
Walk down from a virtual method to overriding methods, we rename them as a group.
Note that canonicalRenameDecl() ensures we're starting from the base method.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132797