nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/ClangdServer.cpp:365
Callback<tooling::Replacements> CB) {
- auto Action = [Sel](decltype(CB) CB, std::string File,
- std::string TweakID,
- Expected<InputsAndAST> InpAST) {
+ auto Action = [Sel](decltype(CB) CB, std::string File, std::string TweakID,
+ Expected<InputsAndAST> InpAST) {
----------------
kadircet wrote:
> Can you revert this change?
I tried, but clang-format automatically makes the change again.
Is there some particular clang-format configuration I should apply, so it
produces the same results? I don't currently have any configuration, i.e. I
think it's just reading the .clang-format file in the monorepo root.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:885
+static Optional<TypeHierarchyItem>
+getTypeHierarchy(const CXXRecordDecl &CXXRD, ASTContext &ASTCtx, int Levels,
+ TypeHierarchyDirection Direction) {
----------------
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.
================
Comment at: clang-tools-extra/unittests/clangd/XRefsTests.cpp:1562
+ const CXXRecordDecl *RD = findRecordTypeAt(AST, Pt);
+ EXPECT_EQ(nullptr, RD);
+ }
----------------
kadircet wrote:
> Can you put a TODO?
I added a comment to clarify that a field does not unambiguously specify a
record type. i don't think there's anything further "to do" here.
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
