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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
