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

[Tom Praschan via Phabricator via cfe-commits]

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