This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Store the containing symbol for refs
ClosedPublic

Authored by nridge on Oct 18 2020, 10:24 PM.

Details

Summary

This is needed to implement call hierarchy.

Diff Detail

Event Timeline

nridge created this revision.Oct 18 2020, 10:24 PM
nridge requested review of this revision.Oct 18 2020, 10:24 PM
kadircet added inline comments.Oct 20 2020, 2:03 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
349

What does ASTNode.Parent correspond to in here? I couldn't find any clarifications on libIndex side, are we sure that's always what we want? It would be nice to have some tests demonstrating what this corresponds to in a variety of cases.

Also why do we only store NamedDecls as containers? It makes sense from CallHierarchy perspective as we are only interested in function-like containers, and they are nameddecls (?). But these might as well be TranslationUnitDecl (?) for top level declarations,

nridge updated this revision to Diff 300518.Oct 24 2020, 6:47 PM

Use Decl rather than NamedDecl, and add a test

nridge marked an inline comment as done.Oct 24 2020, 6:49 PM
nridge added inline comments.
clang-tools-extra/clangd/index/SymbolCollector.cpp
349

I wrote a test with various cases I could think of, and the choice of decl coming from ASTNode.Parent seems reasonable to me.

nridge marked an inline comment as done.
kadircet added inline comments.Oct 29 2020, 10:48 AM
clang-tools-extra/clangd/index/Ref.h
91

i am afraid we are going to have an indeterminate value here if for whatever reason Container detection fails. (e.g. for macros, or if ASTNode.Parent set to nullptr by libindex).

Sent out https://reviews.llvm.org/D90397 to address the issue.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
779

can we also have an assertion checking containers for top level symbols are the same ?

and extend the test to have multiple declarations inside:

  • a namespace
  • inside a record (like S1)

and ensure the container is same for those (and different than top-level symbols themselves). It is okay for those symbols.

Note that it is OK for namespaces to be not in the index, i just want to make sure the containers for the top-level symbols inside a namespace are the same.

nridge updated this revision to Diff 302121.Oct 31 2020, 11:32 PM
nridge marked an inline comment as done.

Address review comments

clang-tools-extra/clangd/index/Ref.h
91

Good catch, and thank you for addressing this!

kadircet accepted this revision.Nov 2 2020, 4:23 AM

thanks, LGTM!

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
782

can you also assert containers here are non-null (and below)

803

nit: i would suggest writing these in the form of:

auto Container = [](StringRef RangeName){
  auto Ref = findRefWithRange(RangeName;
  ASSERT_TRUE(Ref);
  ASSERT_FALSE(Ref->Container.isNull());
  return Ref->Container;
};
EXPECT_EQ(Container("sym1"), Container("sym2"));
EXPECT_NE(Container("sym1"), Container("sym2"));

then you could update the ones above as:

EXPECT_EQ(Container("rangename"), findSymbol(Symbols, "symname"));
This revision is now accepted and ready to land.Nov 2 2020, 4:23 AM
nridge marked an inline comment as done.Nov 2 2020, 11:17 PM
nridge added inline comments.
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
782

It looks like the container is null for toplevel decls. Is that a problem?

nridge updated this revision to Diff 302471.Nov 2 2020, 11:28 PM

Address review comments

(I will wait for a response about the containers for top-level decls before committing.)

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
803

Revised as suggested, except for allowing a null container for top-level decls.

kadircet accepted this revision.Nov 3 2020, 12:09 PM
kadircet added inline comments.
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
770

nit: instead of making this part of the lambda and complicating the signature I would just check this holds for classscope1 and namespacescope1 explicitly. it should be true for other cases anyways, as we are checking equality with non-null smybolids.

782

I think that's OK for now, might be worth leaving a comment tho. (at Ref::Container)

nridge updated this revision to Diff 302759.Nov 4 2020, 12:20 AM
nridge marked 4 inline comments as done.

Address final review comments

This revision was landed with ongoing or failed builds.Nov 4 2020, 12:21 AM
This revision was automatically updated to reflect the committed changes.