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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 marked 4 inline comments as done.
jansvoboda11 added a comment.

In D112915#3136492 <https://reviews.llvm.org/D112915#3136492>, @vsapsai wrote:

> 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?

I think moving `isImport` into `Preprocessor` can be done in a similar way to how we handle the number of includes (or rather "has been included/imported" behavior), so we should be able to reuse some parts of this patch there.

One question that remains to be answered is whether to keep two separate `DenseSet<const FileEntry *>` instances, or merge them into something like:

  struct IncludeInfo {
    bool IsImportedOrIncluded; // currently Preprocessor::IncludedFiles
    bool IsImported;           // currently HeaderFileInfo::isImport
  }
  llvm::DenseMap<const FileEntry *, IncludeInfo> IncludedFilesInfo;

Thanks a lot for the feedback, I appreciate it.



================
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;
----------------
vsapsai wrote:
> 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.
Yeah, spelling the type out everywhere is a bit unwieldy. It's a bit better with `llvm::DenseSet<const FileEntry *>` in the latest revision, but still not pretty. I wanted to avoid pulling `Preprocessor.`h everywhere, but will probably reconsider doing that.


================
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;
+  };
----------------
vsapsai wrote:
> 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.
Thanks a ton, this must've taken quite a bit of time to put together. I agree your approach is much simpler. I'll investigate how it behaves on larger projects, but probably will end up adopting it in this patch.

I have done some minor tweaks locally to fix the failing PCH tests, `Modules/import-textual-noguard.mm` doesn't make much sense to me, I think I'll end up updating that test.


================
Comment at: clang/lib/Lex/Preprocessor.cpp:1336
+      [&](Module *M) {
+        const auto *Includes = getLocalSubmoduleIncludes(M);
+        if (!Includes)
----------------
vsapsai wrote:
> If I drop checking `getLocalSubmoduleIncludes`, no tests are failing. But it seems like this call is required. How can we test it?
I think this should kick in when importing a submodule from the same module. I'll try to come up with a test case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112915