jansvoboda11 added a comment.
In D112915#3104873 <https://reviews.llvm.org/D112915#3104873>, @vsapsai wrote:
> There can be other reasons to keep `IncludeMap` out of `SubmoduleState` but
> I'm not sure the local submodule visibility is the right reason. I might be
> reading the code incorrectly but looks like `CurSubmoduleState` is used when
> local submodule visibility is disabled. The difference is it's tracking the
> aggregate state instead of per-submodule state. Need to experiment more but
> so far tracking includes in `SubmoduleState` follows the spirit of local
> submodule visibility. Though it's not guaranteed it'll work perfectly from
> the technical perspective.
Yes, `CurSubmoduleState` is being used unconditionally. However, without local
submodule visibility enabled, it always points to `NullSubmoduleState`. Only
with the feature enabled does it point to the current submodule state (stored
in `Submodules`). The change happens in `Preprocessor::{Enter,Leave}Submodule`.
> Also I think we'll need to increase granularity to track other HeaderFileInfo
> attributes, not just NumIncludes. Don't have a test case to illustrate that
> right now and no need to change that now but something to keep in mind.
That's interesting. I think `HeaderFileInfo::isImport` should definitely be
tracked in the preprocessor, not in `HeaderFileInfo`. The fact that the header
was `#import`ed is not an intrinsic property of the file itself, but rather a
preprocessor state. Can you think of other fields that don't really belong to
`HeaderFileInfo`?
Based on your feedback, I simplified the patch quite a bit. We're no longer
copying the include state between submodules. In its current form, this patch
essentially moves `HeaderFileInfo::NumIncludes` into
`Preprocessor::NumIncludes` and still uses it as the source of truth.
However, we're also tracking `NumIncludes` separately in each submodule and
serializing this into the PCM. Instead of merging `NumIncludes` of the whole
module when loading it (which we did before), we can merge `NumIncludes` only
of the modules we actually import.
================
Comment at: clang/lib/Lex/Preprocessor.cpp:1323
+ auto &ModNumIncludes = SubmoduleIncludeStates[M].NumIncludes;
+ for (unsigned UID = 0; UID <= ModNumIncludes.maxUID(); ++UID) {
+ CurSubmoduleIncludeState->NumIncludes[UID] += ModNumIncludes[UID];
----------------
Iterating over all FileEntries is probably not very efficient, as Volodymyr
mentioned. Thinking about how to make this more efficient...
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
