[PATCH] D78038: [clangd] WIP: fix several bugs relating to include insertion

Mon Jul 17 06:02:24 PDT 2023

sammccall added a comment.

OK, time to revive this patch, sorry for letting it die.

Meanwhile, the lexer change has landed separately, so that's gone.

I've removed the gratuitous clangd indexing changes in order to focus this on the serialization hacks.

================
Comment at: clang/lib/Lex/Lexer.cpp:2749
+    // most useful answer is "yes, this file has a header guard".
+    if (!ConditionalStack.empty())
+      MIOpt.ExitTopLevelConditional();
----------------
kadircet wrote:
> I think we should put this behind a PPOpt to make sure we don't regress the rest of the world, as I fear this part of the code might not be well tested.
(this is obsolete as this code landed separately)

================
Comment at: clang/lib/Serialization/ASTReader.cpp:6155
+      // To avoid doing this on every miss, require the bare filename to match.
+      if (Pos == Table->end() && M.Kind == clang::serialization::MK_Preamble &&
+          llvm::sys::path::filename(FE->getName()) ==
----------------
kadircet wrote:
> do we have other (apart from `isFileMultiIncludeGuarded`) things that depend on HFI for main file being correctly deserialized?
> 
> If it is only the include guard, maybe we can store that info in control block and later on restore it when reading HFI for main file in `HeaderSearch::getExistingFileInfo`?
The real answer here is "I have no idea". I definitely don't have bug reports saying other things are broken.
HFI looks like it affects module semantics, so seems like it could be relevant when the main file is part of a module. But today we don't have modules support...

Really, I'd just rather make the existing mechanism work than add a second one.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78038