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