tom-anders added inline comments.
================
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);
----------------
tom-anders wrote:
> 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
Nope, I totally misunderstood, should be better now
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
