dexonsmith marked 2 inline comments as done. dexonsmith added inline comments.
================ Comment at: clang/lib/Frontend/FrontendActions.cpp:182 + CI.getFrontendOpts().BuildingImplicitModule && + CI.getLangOpts().isCompilingModule())); Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator( ---------------- dexonsmith wrote: > jordan_rose wrote: > > Why is this the condition, as opposed to just "do this for all modules, > > don't do it for PCHs"? And doesn't `BuildingImplicitModule` imply > > `isCompilingModule()`? (Note that `BuildingImplicitModule` probably isn't > > the same as the original condition `ImplicitModules`.) > > (Note that BuildingImplicitModule probably isn't the same as the original > > condition ImplicitModules.) > > Nice catch; I ported the logic from `ASTWriter` incorrectly. I'll fix this. > > > Why is this the condition, as opposed to just "do this for all modules, > > don't do it for PCHs"? And doesn't BuildingImplicitModule imply > > isCompilingModule()? > > I was cargo-culting the logic for how `PCHGenerator::HandleTranslationUnit` > sets the `Module` parameter to `ASTWriter::WriteAST`. I think that if I fix > the above to match the original condition I'll need to check > `isCompilingModule()`... but maybe `BuildingImplicitModule` is already > `&&`-ing these together? I'll check. Updated the patch to just use `BuildingImplicitModule`. The previous condition in `PCHGenerator::HandleTranslationUnit` seems to have been equivalent. - Previously in `PCHGenerator::HandleTranslationUnit`, `WritingModule` would be non-null only if we're currently building a module, as opposed to writing out a PCH. The logic to write to the in-memory cache also checked if `LangOptions::ImplicitModules` was set. - Now in `GenerateModuleAction::CreateASTConsumer` we check `BuildingImplicitModule` which is set in `compileModuleImpl` before executing the action. I'm choosing this because it better matches the code around. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59176/new/ https://reviews.llvm.org/D59176 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
