[PATCH] D128457: [clangd] Add new IncludeType to IncludeHeaderWithReferences

[David Goldman via Phabricator via cfe-commits]

dgoldman marked 4 inline comments as done.
dgoldman added a comment.

Also LMK if you want more in this change (such as a flag to control it, just not sure where it should live + what it should be called).



================
Comment at: clang-tools-extra/clangd/index/Symbol.h:116
+    /// this header.
+    IncludeTypes IncludeTypes = Invalid;
   };
----------------
kadircet wrote:
> the naming here feels a little confusing, can we change the enum name to be `IncludeDirective` and field name to `SupportedDirectives`
Done, although for Serialization I left it as a 32 bit and then 8 bit include directives, LMK if I should swap it over to serialize as a 32 bit single value... Guess I would need to use a union or manually bit manipulate it to store + load it?


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1689-1690
   auto TU = TestTU::withHeaderCode("int x();");
-  EXPECT_THAT(TU.headerSymbols(), ElementsAre(includeHeader()));
+  // FIXME(davg): This doesn't work since we use a -include to put a sneaky #import in.
+  // EXPECT_THAT(TU.headerSymbols(), ElementsAre(includeHeader()));
 
----------------
Not sure the best way to fix this, should we also scan the -include files for #imports?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128457