[PATCH] D94382: [clangd] Avoid recursion in TargetFinder::add()

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Doh, I really thought we'd get away without an explicit recursion guard here.
But the example is compelling, so this seems like the right approach. Unfortunately, I think we're going to end up needing to add allocations too...



================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:333
       Decls;
+  const Decl *CurrentlyProcessing = nullptr;
   RelSet Flags;
----------------
I don't think a single pointer is going to cut it here - can't we construct a mutually-recursive example so that the "wrong" decl is on always on top of the stack and we never bail out?

We probably want a `SmallDenseMap Seen` or something? (And then we no longer need to look up in the actual result set).

Couple of related refinements:
 - Currently when we reach the same node via multiple flag paths, we take the union of the flags. (I'm not sure this is *great* behavior, but the API is pretty limiting). To be consistent with that here, we should store the flags for `seen` too, and union them when updating, and only bail out if we added no new flags. (This will almost never matter in practice, but it'd be nice not to worry)
 - You could consider making the key type `void*` instead of `decl*` and also break loops with no decls in them. But this can certainly wait until we find such a loop!


================
Comment at: clang-tools-extra/clangd/FindTarget.cpp:333
       Decls;
+  const Decl *CurrentlyProcessing = nullptr;
   RelSet Flags;
----------------
nridge wrote:
> njames93 wrote:
> > Whats the purpose in tracking this? Is the Decls lookup not enough?
> No, because the possibly-recursive call is [here](https://searchfox.org/llvm/rev/9f2d9364b04c4d9651b1ec8612a3968b969fe71d/clang-tools-extra/clangd/FindTarget.cpp#366), but we only insert into `Decls` [here](https://searchfox.org/llvm/rev/9f2d9364b04c4d9651b1ec8612a3968b969fe71d/clang-tools-extra/clangd/FindTarget.cpp#417).
> No, because the possibly-recursive call is here, but we only insert into Decls here.

This is just for implementation convenience, we could reverse the order by rearranging the code somehow. However this seems fragile to me - I'm thinking about the cases where we reassign D in the function of the body here, and so end up reporting some other declaration instead (which may not be in the cycle).
So I think I like the solution in this patch better.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94382