vsapsai added a comment. I'm not going to cover the entire change, some parts I need to consider more carefully.
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. 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. ================ Comment at: clang/lib/Lex/PPLexerChange.cpp:699 + // current. + for (const auto& Includes : CurIncludeMap->OuterIncludes) + IncludeState.OuterIncludes[Includes.getFirst()] += Includes.getSecond(); ---------------- How many includes are expected to be here? Are this only immediate includes or also transitive? Asking to evaluate how expensive iterating through the includes can get. 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
