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

[Sam McCall via Phabricator via cfe-commits]

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