rsmith added a comment.
I wonder if perhaps we're tracking this state in the wrong way. The "has been
included" information for `#pragma once` / `#import` should behave exactly like
macro definition visibility: it should be reset whenever we enter a new "clean
slate" state and should be saved and restored suitably when switching
visibility states. If we track, for each header file, a map from module to
include information, then we'll be paying a fairly high cost: each time we see
a `#include` we'll need to look up every currently-visible module in the map to
see if the header was included in that module. It might be better to track an
up-to-date list of the headers that have been either `#import`ed in the current
preprocessor state (including via imported modules) or that have been both
`#include`d and contain `#pragma once`. You could stash that in the
`Preprocessor::SubmoduleState` so that it's suitably swapped out when we switch
to a new preprocessing state.
================
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;
+
----------------
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.)
================
Comment at: clang/include/clang/Serialization/ASTBitCodes.h:44
/// AST files at this time.
-const unsigned VERSION_MAJOR = 15;
+const unsigned VERSION_MAJOR = 16;
----------------
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?)
================
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;
+ }
----------------
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.
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
