Page MenuHomePhabricator

[clangd] SymbolCollector support for relations
ClosedPublic

Authored by nridge on May 26 2019, 4:48 PM.

Details

Summary

The only relation currently collected is RelationBaseOf, because this is
all we need for type hierarchy subtypes. Additional relations can be
collected in the future as the need arises.

This patch builds on D59407 and D62459.

Diff Detail

Repository
rL LLVM

Event Timeline

nridge created this revision.May 26 2019, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2019, 4:48 PM

Can you also add some tests ?

we have some tests in unittests/SymbolCollectorTests.cpp

clang-tools-extra/clangd/index/SymbolCollector.cpp
298 ↗(On Diff #201473)

why do we want to process these relations for references?

426 ↗(On Diff #201473)

maybe rather check TagDecl to be as generic as possible.

also the check can be inlined since you are not using RD afterwards, i.e

if(!dyn_cast<TagDecl>(&ND))
  return;
431 ↗(On Diff #201473)

this might get complicated, maybe isolate it to a bool shouldIndexRelation(const Relation&)

434 ↗(On Diff #201473)

maybe call it Object ?

436 ↗(On Diff #201473)

why? I believe we have those in the index since rL356125.

even if we don't I believe this part should also be isolated to somehow get canonical version of a given symbol.

441 ↗(On Diff #201473)

nit: invert the condition to reduce nesting

453 ↗(On Diff #201473)

I believe subject is ParentID and object is ID in a baseof relation so these should be replaced ?

453 ↗(On Diff #201473)

also we should generalize this with something like index::applyForEachSymbolRole, which should also get rid of the check at top(shouldIndexRelation)

nridge updated this revision to Diff 202371.May 30 2019, 10:35 PM
nridge marked 11 inline comments as done.

Addressed most review comments

nridge added inline comments.May 30 2019, 10:36 PM
clang-tools-extra/clangd/index/SymbolCollector.cpp
298 ↗(On Diff #201473)

The RelationBaseOf is not present in the Relations parameter of the handleDeclOccurrence() call for the base class's declaration. It's only present for the specific reference that's in the base-specifier.

436 ↗(On Diff #201473)

I removed this logic as it no longer seems to be necessary now that we use libIndex's relations support to compute our relations.

453 ↗(On Diff #201473)

I believe subject is ParentID and object is ID in a baseof relation so these should be replaced ?

The code was functionally correct, but the name ParentID was wrong -- the RelatedSymbol for a BaseOf relation is the child.

Anyways, I'm now calling it ObjectID.

453 ↗(On Diff #201473)

also we should generalize this with something like index::applyForEachSymbolRole, which should also get rid of the check at top(shouldIndexRelation)

I don't fully understand this suggestion; could you elaborate?

Are you thinking ahead to a future where we want to index additional kinds of relations? Can we refactor things at that time?

kadircet added inline comments.May 31 2019, 1:33 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
441 ↗(On Diff #202371)

nit: braces

298 ↗(On Diff #201473)

yeah just checked the indexing code, that's unfortunate :/.

could you add a comment explaining the situation ?

431 ↗(On Diff #201473)

nit: no need for braces

453 ↗(On Diff #201473)

yes I was thinking ahead. just wanted to make sure we can add other relation types without changing the logic inside processrelations.

but it is not that important, let's keep it that way, we can refactor it later on.

nridge updated this revision to Diff 202646.Jun 2 2019, 10:06 PM
nridge marked 5 inline comments as done.

Address remaining review comments

kadircet accepted this revision.Jun 3 2019, 1:47 AM

LGTM, thanks!

I suppose there are only two patches left now?

  • Exposing relations in SymbolIndex.
  • Implementing children resolution using that information.
This revision is now accepted and ready to land.Jun 3 2019, 1:47 AM
nridge added a comment.Jun 3 2019, 8:58 PM

LGTM, thanks!

Thanks for the reviews so far!

I suppose there are only two patches left now?

  • Exposing relations in SymbolIndex.
  • Implementing children resolution using that information.

Yup. Coming up in the near future :)

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2019, 9:24 PM