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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
