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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov 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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132797