[PATCH] D61442: [clangd] Fix header-guard check for include insertion, and don't index header guards.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri May 3 05:38:19 PDT 2019
sammccall added inline comments.
================
Comment at: clangd/index/SymbolCollector.h:130
+ // The final spelling is calculated in finish().
+ llvm::DenseMap<SymbolID, FileID> IncludeFiles;
+ // Indexed macros, to be erased if they turned out to be include guards.
----------------
kadircet wrote:
> This is losing ability to store multiple header files. Is that intentional?
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 :-)
================
Comment at: clangd/unittests/SymbolCollectorTests.cpp:1043
+ int decl();
+ #define MACRO
+
----------------
kadircet wrote:
> can you also add a case where we first see `MACRO` and then `decl`.
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.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D61442/new/
https://reviews.llvm.org/D61442
More information about the cfe-commits
mailing list