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

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:548
+namespace {
+void recursivelyInsertOverrides(const SymbolID &Base,
+                                llvm::DenseSet<SymbolID> &IDs,
----------------
sammccall wrote:
> 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.
> ```
nit: can pass SymbolID by value, it's 8 bytes


================
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:
> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132797