This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add SymbolID to LocatedSymbol.
ClosedPublic

Authored by usaxena95 on Apr 27 2021, 11:48 AM.

Details

Summary

This is useful for running in batch mode.
Getting the SymbolID from via getSymbolInfo may give SymbolID
of a symbol different from that located by LocateSymbolAt (they
have different semantics of choosing the symbol.)

Diff Detail

Event Timeline

usaxena95 created this revision.Apr 27 2021, 11:48 AM
usaxena95 requested review of this revision.Apr 27 2021, 11:48 AM
sammccall accepted this revision.Apr 28 2021, 2:41 AM

LG, but can you amend the description a bit?

We do not have to both locate the symbol and also query the SymbolID (using getSymbolInfo).

IIRC this isn't really the reason, it's because getSymbolInfo and locateSymbolAt don't have the same semantics *for choosing symbols* and we want the ID of the symbol that matches the latter.

clang-tools-extra/clangd/XRefs.cpp
260

note that this populates SymbolID even if there's no ID available, so it's incorrect if the type is Optional<SymbolID>

clang-tools-extra/clangd/XRefs.h
51

There are other cases too, maybe just "if available"

52

SymbolID is already inherently optional (it has a zero value for a whil enow)

clang-tools-extra/clangd/unittests/XRefsTests.cpp
924

nit, just EXPECT_TRUE(Results[0].ID), and remove the matcher?

This revision is now accepted and ready to land.Apr 28 2021, 2:41 AM
usaxena95 edited the summary of this revision. (Show Details)Apr 28 2021, 5:56 AM
usaxena95 updated this revision to Diff 341168.Apr 28 2021, 5:57 AM
usaxena95 marked 4 inline comments as done.
usaxena95 edited the summary of this revision. (Show Details)

Addressed comments.

usaxena95 edited the summary of this revision. (Show Details)Apr 28 2021, 5:58 AM
This revision was landed with ongoing or failed builds.Apr 28 2021, 6:06 AM
This revision was automatically updated to reflect the committed changes.