[PATCH] D59176: Modules: Add LangOptions::CacheGeneratedPCH

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith marked 2 inline comments as done.
dexonsmith added a comment.

In D59176#1424864 <https://reviews.llvm.org/D59176#1424864>, @jordan_rose wrote:

> This commit by itself doesn't change any behavior, right?


Correct, it just exposes an option.



================
Comment at: clang/lib/Frontend/FrontendActions.cpp:115
       CI.getPreprocessorOpts().AllowPCHWithCompilerErrors,
-      FrontendOpts.IncludeTimestamps));
+      FrontendOpts.IncludeTimestamps, +CI.getLangOpts().CacheGeneratedPCH));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
----------------
jordan_rose wrote:
> What's the `+` for?
Heh, I was trying to figure out those `+`s as well until I added this without one and it didn't compile.  The problem is that bitfields can't be forwarded through the `llvm::make_unique` variadic.  The `+` creates a local temporary with the same value.


================
Comment at: clang/lib/Frontend/FrontendActions.cpp:182
+      CI.getFrontendOpts().BuildingImplicitModule &&
+          CI.getLangOpts().isCompilingModule()));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
----------------
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59176/new/

https://reviews.llvm.org/D59176