vsapsai added a comment.
That's a good point. Let me check how we track macros, I haven't thought about
that approach. And I haven't considered using `Preprocessor::SubmoduleState`,
was too excited `HeaderSearch::ShouldEnterIncludeFile` works correctly with the
updated data.
================
Comment at: clang/include/clang/Lex/HeaderSearch.h:126
+ /// Bool represents if the header was included (false) or imported (true).
+ llvm::DenseMap<const Module *, bool> ModuleIncludersMap;
+
----------------
rsmith wrote:
> This seems like a very expensive thing to include in `HeaderFileInfo`, which
> we may have many thousands of in some compilations. (I have similar concerns
> about `Aliases` FWIW.)
My fault. The fact that the struct is tightly packed, should have been a hint.
================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:44
/// AST files at this time.
-const unsigned VERSION_MAJOR = 15;
+const unsigned VERSION_MAJOR = 16;
----------------
rsmith wrote:
> Do we really need to bump the version number? (Does this affect something we
> read before we read the Clang version number and reject on a version
> mismatch?)
Nope, there is no logic for VERSION_MAJOR. Just wanted to avoid old clang
writing HeaderFileInfoTrait in one format and new clang reading it in the new
format. But if that is resolved by Clang version number, no need for
VERSION_MAJOR bump.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:1912-1916
+ // Update isImport and NumIncludes based on including module.
+ if (Mod->NameVisibility == Module::NameVisibilityKind::AllVisible) {
+ HFI.isImport |= IsImport;
+ ++HFI.NumIncludes;
+ }
----------------
rsmith wrote:
> This doesn't do the right thing in local submodule visibility mode. We need
> to compute visibility locally from within `HeaderSearch` based on the
> currently visible modules at the point of the query instead.
Is it wrong from general local submodule visibility approach or do you have a
test case in mind? Don't need a specific case, general direction can help to
come up with a test case.
In practice it was working because we were reading this part of PCM file only
when HeaderFileInfo was requested and that was at the point when modules had
correct visibility. Let me try with multiple unmodularized headers, that can be
broken and it can be a good test case on its own.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D104344/new/
https://reviews.llvm.org/D104344
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
