[PATCH] D112915: WIP: [clang][modules] Granular tracking of includes

Tue Nov 2 19:17:47 PDT 2021

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