nridge marked an inline comment as done.
nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional<TypeHierarchyItem>
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+ TypeHierarchyDirection Direction) {
----------------
nridge wrote:
> sammccall wrote:
> > kadircet wrote:
> > > nridge wrote:
> > > > sammccall wrote:
> > > > > The scope and complexity of this function seems unneccesarily large:
> > > > > - it's (in future?) capable of resolving in both directions
> > > > > - it calls itself recursively, modifying TypeHierarchyDirection to
> > > > > control the search
> > > > > - it handles levels, even though this optimization is only important
> > > > > for derived types
> > > > >
> > > > > Can we restrict this to retrieving (all) recursive base types?
> > > > > i.e. `Optional<TypeHierarchyItem> getTypeAncestors(const
> > > > > CXXRecordDecl &CXXRD, ASTContext &Ctx)`
> > > > > Then later, subtype resolution can be the job of another function:
> > > > > `resolveTypeDescendants(TypeHierarchyItem& Item, int depth)`
> > > > >
> > > > > That way the recursion of getTypeAncestors is simpler to understand,
> > > > > as it has much smaller scope. And code sharing between the two LSP
> > > > > calls is clearer: fetching type hierarchy is a call to
> > > > > `getTypeAncestors` and a call to `resolveTypeDescendants` (if
> > > > > direction is children or both, and resolve is > 0), and resolving a
> > > > > hierarchy is a call to `resolveTypeDescendants`.
> > > > If we remove "levels" here, should we introduce some kind of guard for
> > > > infinite recursion, in case the user writes something like:
> > > >
> > > > ```
> > > > template <int N>
> > > > struct S : S<N + 1> {};
> > > >
> > > > S<0> s;
> > > > ```
> > > clang should be limiting recursive template instantiations. Also since we
> > > are just traversing the AST produced by clang, it should never be
> > > infinite, but still a nice test case can you add one?
> > I think there is a point here...
> >
> > Consider our `S` template with the direction reversed and a base case
> > specialized:
> > ```
> > template <int N>
> > struct S : S<N - 1> {};
> > template<>
> > struct S<0>{};
> >
> > S<2> instance;
> > ```
> >
> > Now the hierarchy of `S<2>` is well defined and finite: `S<2> : S<1> :
> > S<0>`.
> > However IIUC the `CXXRecordDecl` for `S<2>` is the instantiation of the
> > primary template, whose base is `S<N - 1>`, which is dependent[1], so the
> > base's `CXXRecordDecl` is the primary template, whose base is `S<N - 1>`...
> > and we never reach the base case.
> >
> > Actually I'm not sure whether this happens if the base is dependent merely
> > where it's spelled, or still dependent after instantiation? Even in the
> > latter case one can construct examples where we'll infloop:
> > ```template <int N>
> > struct Foo {
> > S<N> instance;
> > }```
> > Trying to get the hierarchy on `S<N>` could infloop. (I agree these should
> > both be test cases)
> >
> > What's the Right Thing?
> > - not sure about a recursion limit, as showing 10 `S<N-1>`s in the
> > hierarchy is silly.
> > - not sure about bailing out on dependent types either, as knowing that
> > e.g. my `SmallSet<T>` inherits from `SmallMap<T, bool>` is meaningful or
> > useful.
> > - maybe we should bail out once we see instantiations of the same template
> > decl twice in a parent chain. I.e. keep the seen non-null
> > `CXXRecordDecl->getTemplateInstantiationPattern()`s in a set and stop
> > recursing if insertion fails.
> >
> > However for this patch I'd suggest just bailing out on dependent types with
> > a TODO as the simplest, this is an edge case that can be fixed later.
> >
> The current patch does actually recurse infinitely on this test case, likely
> for the reasons Sam outlined (our handling of dependent types), and
> eventually runs into the segfault.
>
> I will update the patch to address this.
I meant to say "eventually runs into a stack overflow".
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D56370/new/
https://reviews.llvm.org/D56370
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
