This is an archive of the discontinued LLVM Phabricator instance.

[Clangd] Index main-file macros (bug 39761)
ClosedPublic

Authored by nridge on Dec 15 2018, 2:55 PM.

Diff Detail

Event Timeline

nridge created this revision.Dec 15 2018, 2:55 PM
nridge added subscribers: sammccall, hokein.

@hokein, @sammccall, this is a follow-up to https://reviews.llvm.org/D55185. Would one (or both) of you be intereested in reviewing it?

sammccall accepted this revision.Jan 21 2019, 8:25 AM
sammccall added inline comments.
clang-tools-extra/clangd/index/SymbolCollector.cpp
385 ↗(On Diff #181957)

why do we check IsMainFileSymbol here?

This revision is now accepted and ready to land.Jan 21 2019, 8:25 AM
hokein added inline comments.Jan 22 2019, 1:41 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
413 ↗(On Diff #181957)

I think we need to set VisibleOutsideFile flag.

nridge updated this revision to Diff 183020.Jan 22 2019, 7:08 PM

Address review comments

nridge marked 4 inline comments as done.Jan 22 2019, 7:08 PM
nridge added inline comments.
clang-tools-extra/clangd/index/SymbolCollector.cpp
385 ↗(On Diff #181957)

Good point, it doesn't seem to be necessary.

413 ↗(On Diff #181957)

Good catch! Fixed and amended tests.

Just realize one missing point: we should respect the CollectMainFileSymbols option, otherwise looks good.

nridge updated this revision to Diff 183319.Jan 24 2019, 8:05 AM
nridge marked 2 inline comments as done.

Address more review comments

nridge updated this revision to Diff 183320.Jan 24 2019, 8:06 AM

Don't include unrelated file

Just realize one missing point: we should respect the CollectMainFileSymbols option, otherwise looks good.

Good catch once again -- fixed.

hokein accepted this revision.Jan 28 2019, 3:04 AM
hokein added a comment.EditedJan 28 2019, 6:03 AM

Measured this patch on LLVM, the increasing number of symbols is reasonable (from 422 K to 425 K, with CollectMacro enabled).

This revision was automatically updated to reflect the committed changes.