[PATCH] D112915: [clang][modules] Track number of includes per submodule

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

Didn't go in-depth for serialization/deserialization. When we add tracking `isImport` on per-submodule basis, do you think AST reading/writing would change significantly?



================
Comment at: clang/include/clang/Lex/ExternalPreprocessorSource.h:45
+  /// Return the files directly included in the given (sub)module.
+  virtual const llvm::DenseMap<const FileEntry *, unsigned> *
+  getIncludedFiles(Module *M) = 0;
----------------
I think it is better for understanding and more convenient to use some `using` instead of duplicating `llvm::DenseMap<const FileEntry *, unsigned>` in multiple places.


================
Comment at: clang/include/clang/Lex/Preprocessor.h:775-781
+  struct SubmoduleIncludeState {
+    /// For each included file, we track the number of includes.
+    llvm::DenseMap<const FileEntry *, unsigned> IncludedFiles;
+
+    /// The set of modules that are visible within the submodule.
+    VisibleModuleSet VisibleModules;
+  };
----------------
I think the interplay between `CurSubmoduleIncludeState`, `IncludedFiles`, and `CurSubmoduleState` is pretty complicated. Recently I've realized that it can be beneficial to distinguish include tracking for the purpose of serializing per submodule and for the purpose of deciding if should enter a file. In D114051 I've tried to illustrate this approach. There are some tests failing but hope the main idea is still understandable.

One of my big concerns is tracking `VisibleModules` in multiple places. D114051 demonstrates one of the ways to deal with it but I think it is more important for you to know the problem I was trying to solve, not the solution I came up with.


================
Comment at: clang/lib/Basic/Module.cpp:653
     ImportLocs[ID] = Loc;
-    Vis(M);
+    Vis(V.M);
 
----------------
Was meaning to make this fix for a long time but couldn't test it. Thanks for finally fixing it!


================
Comment at: clang/lib/Lex/Preprocessor.cpp:1336
+      [&](Module *M) {
+        const auto *Includes = getLocalSubmoduleIncludes(M);
+        if (!Includes)
----------------
If I drop checking `getLocalSubmoduleIncludes`, no tests are failing. But it seems like this call is required. How can we test it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112915