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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
