[PATCH] D112915: [clang][modules] Track number of includes per submodule

dexonsmith added inline comments. Comment at: clang/include/clang/Serialization/ModuleFile.h:399 + /// include information. + llvm::StringMap> SubmoduleIncludedFiles; + jansvoboda11 wrote: > dexonsmith wrote: > > Each StringMapEntry is going to have a pretty bi

[PATCH] D112915: [clang][modules] Track number of includes per submodule

jansvoboda11 marked 4 inline comments as done. jansvoboda11 added a comment. In 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/w

[PATCH] D112915: [clang][modules] Track number of includes per submodule

jansvoboda11 marked 5 inline comments as done. jansvoboda11 added a comment. In D112915#3122340 , @dexonsmith wrote: > Implementation looks a lot cleaner! > > I'd still like to drop NumIncludes first if possible because I think it'll be > easier to reas

[PATCH] D112915: [clang][modules] Track number of includes per submodule

jansvoboda11 updated this revision to Diff 387952. jansvoboda11 added a comment. Rebase on top of extracted patches. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112915/new/ https://reviews.llvm.org/D112915 Files: clang/include/clang/Lex/Extern

[PATCH] D112915: [clang][modules] Track number of includes per submodule

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 + /// Retur

[PATCH] D112915: [clang][modules] Track number of includes per submodule

vsapsai added a comment. As we've discussed earlier, tracking `isImport` shouldn't be done per .pcm and here is the test case https://gist.github.com/vsapsai/a2d2bd19c54c24540495fd9b262106aa I'm not sure it is worth adding the second `#include` as the test fails just with one. Overall, the cha

[PATCH] D112915: [clang][modules] Track number of includes per submodule

dexonsmith added a comment. Implementation looks a lot cleaner! I'd still like to drop NumIncludes first if possible because I think it'll be easier to reason about without this extra layer of complexity. Also, that'd mitigate the potential regression in `.pcm` size. (Note: I'd be more comfort

[PATCH] D112915: [clang][modules] Track number of includes per submodule

jansvoboda11 updated this revision to Diff 386139. jansvoboda11 added a comment. Store only direct includes in the PCM (compared to transitive stored previously), use InputFile ID (instead of full filesystem path). Also: add test of transitive includes, fix bug in `VisibleModuleSet::setVisible`

[PATCH] D112915: [clang][modules] Track number of includes per submodule

jansvoboda11 updated this revision to Diff 385751. jansvoboda11 marked an inline comment as done. jansvoboda11 added a comment. Make clang-format happy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112915/new/ https://reviews.llvm.org/D112915 Fil

[PATCH] D112915: [clang][modules] Track number of includes per submodule

jansvoboda11 marked 2 inline comments as done. jansvoboda11 added a comment. Thanks for your feedback @dexonsmith. I reworked the patch to use more sensible data structures as suggested, and made the AST deserialization lazy (on (sub)module import). I think the only thing to figure out is the e