[PATCH] D104344: [modules] Track how headers are included by different submodules.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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