tom-anders marked 6 inline comments as done.
tom-anders added inline comments.
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:245
trace::Span Tracer("FindOccurrencesWithinFile");
assert(canonicalRenameDecl(&ND) == &ND &&
"ND should be already canonicalized.");
----------------
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?
================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:554
+ Req.Subjects = {Base};
+ Index.relations(Req, [&](const SymbolID &, const Symbol &Override) {
+ IDs.insert(Override.ID);
----------------
ilya-biryukov wrote:
> sammccall wrote:
> > ilya-biryukov wrote:
> > > Should we put a limit on the number of requests we send during recursion
> > > here?
> > >
> > > I see a few obvious failure modes:
> > > - infinite recursion in the relations due to parts of index being stale,
> > > corrupted input data or other reasons,
> > > - exponential blow up in hierarchies with multiple inheritance,
> > > - sending a lot of network requests in case of deep inheritance
> > > hierarchies for remote index implementations. Since all requests are
> > > sequential, the network latency might add up to substantial numbers.
> > >
> > > We could address these in some other manner, this just seems to be the
> > > simplest option to protect against catastrophic outcomes (running the
> > > request indefinitely, crashing due to infinite recursion, etc).
> > > exponential blow up in hierarchies with multiple inheritance,
> >
> > It seems with little loss of readability we could provide some useful
> > bounds:
> >
> > ```
> > DenseSet<SymbolID> Pending = {Base};
> > while (!Pending.empty()) {
> > Req = {.Subjects = Pending};
> > Pending.clear();
> > Index.relations(Req, { IDs.insert(ID); Pending.insert(ID) });
> > }
> > ```
> > in this case the #requests is clearly bounded by the length of the shortest
> > chain to the most distant SymbolID, and is IMO safe with no numeric cap.
> >
> > whereas the current version could potentially get the same ID in multiple
> > branches and so the bound on #requests is harder to determine.
> This looks good! Also avoids infinite recursion.
> Having a potentially large number of sequential network requests still looks
> unfortunate, but I doubt this will bite us in practice (at least not for
> remote indicies for LLVM and Chrome).
>
> To solve it, we could allow recursive requests and implement the recursion
> inside the index, but this could be done with a follow-up when we actually
> hit this issue.
Done, hope I interpreted your sketch correctly
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
