This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Fix header-guard check for include insertion, and don't index header guards.
ClosedPublic

Authored by sammccall on May 2 2019, 7:38 AM.

Event Timeline

sammccall created this revision.May 2 2019, 7:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2019, 7:38 AM
kadircet accepted this revision.May 3 2019, 4:15 AM

Thanks!

mostly LGTM, only confused about changing symbolcollector to produce a unique header per symbol(where as it was(could) produce multiple headers before).
What is the rationale behind that one?

clangd/index/SymbolCollector.h
130

This is losing ability to store multiple header files. Is that intentional?

clangd/unittests/SymbolCollectorTests.cpp
1043

can you also add a case where we first see MACRO and then decl.

This revision is now accepted and ready to land.May 3 2019, 4:15 AM
sammccall updated this revision to Diff 197967.May 3 2019, 5:38 AM
sammccall marked 3 inline comments as done.

Extract setIncludeLocation() and call it from a more obvious point in the code.
Tweak test to make it harder.

clangd/index/SymbolCollector.h
130

Careful reading of the code shows that ability never existed :-) addDeclaration always creates a new Symbol, sometimes populates its IncludeHeaders, and then replaces the existing symbol. We always find a single decl of the symbol we prefer in the TU (though sometimes it takes us a few attempts).

Of course, I never read the code that carefully - I wrote this as a vector<pair<SymbolID, FileID>> to "preserve" the ability to add multiple headers... and then the tests failed :-)

clangd/unittests/SymbolCollectorTests.cpp
1043

IIUC the idea is that macros are "eager-er" than decls so more likely to break the caching. That makes sense.

WDYT about just putting the macro first, so we *only* test the "hard" case.

kadircet added inline comments.May 3 2019, 5:45 AM
clangd/index/SymbolCollector.h
130

Ah, ok I see. Thanks for pointing this out!

clangd/unittests/SymbolCollectorTests.cpp
1043

exactly, SGTM

This revision was automatically updated to reflect the committed changes.