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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet 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.
----------------
sammccall wrote:
> 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 :-)
Ah, ok I see. Thanks for pointing this out!


================
Comment at: clangd/unittests/SymbolCollectorTests.cpp:1043
+    int decl();
+    #define MACRO
+
----------------
sammccall wrote:
> 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.
exactly, SGTM


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61442/new/

https://reviews.llvm.org/D61442