[PATCH] D85532: Correctly set CompilingPCH in PrecompilePreambleAction.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

Regarding tests, it feels like we can also test this in ASTUnitTests which is directly in clang, as it is also using PrecompiledPreamble::Build. What about moving the test there instead?



================
Comment at: clang-tools-extra/clangd/unittests/ModulesTests.cpp:24
+  TestTU TU = TestTU::withCode(R"cpp(
+#include "Textual.h"
+
----------------
nit: indent the string literals by at least 4 spaces. (that's what other tests look like, same below)


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


================
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]))
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85532