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

Mon Sep 5 05:54:14 PDT 2022

ilya-biryukov added a comment.

Thanks for the patch! A drive-by comment from me, hopefully useful.

================
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);
----------------
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).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132797