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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to