[PATCH] D85532: Correctly set CompilingPCH in PrecompilePreambleAction.

[Adam Czachorowski via Phabricator via cfe-commits]

adamcz added inline comments.


================
Comment at: clang-tools-extra/clangd/unittests/ModulesTests.cpp:28
+)cpp");
+  TU.ExtraArgs.push_back("-fmodules");
+  TU.ExtraArgs.push_back("-fmodule-name=M");
----------------
kadircet wrote:
> this one is not needed right?
I suppose not.


================
Comment at: clang/lib/Frontend/PrecompiledPreamble.cpp:421
       StoreInMemory ? &Storage.asMemory().Data : nullptr, Callbacks));
   Callbacks.BeforeExecute(*Clang);
   if (!Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0]))
----------------
kadircet wrote:
> anything operating on CompilerInstance before and including the callbacks invoked here will see a broken LangOtps. Why not set it here (and maybe assert in the override, or just not do anything at all)
I think we should at least assert() on this, so others don't waste as much time as I did looking for bugs like this. PrecompilePreambleAction is not usable with CompilingPCH = false.

I'm a bit conflicted on setting CompilingPCH outside of PrecompilePreambleAction, it makes it seem like you can change it somehow and it makes that Action class not self-contained (it requires that external bit that sets the options), but with assert it should be fine and it's essentially an internal detail of PrecompilePreamble class anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85532