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