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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
