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

In D69585#1818205 <https://reviews.llvm.org/D69585#1818205>, @rnk wrote:

> I'm interested in making clang do this, but I think this needs significantly 
> more work until this is ready to land. It needs in-tree tests.

What tests specifically? I can add one test that just uses the option and then 
checks the PCH already contains what it should, but besides that this would 
need to repeat all PCH tests with this enabled. Should I just do that?

> I assumed we'd want to hook it up to `clang-cl /Yc` and `/Yu`.

If you mean automatically, then probably not. As said in the description, this 
leads to an error if a PCH'd template has a specialization that's not at least 
mentioned in the PCH. That's not a big deal and it's easy to handle, but it's 
still technically a regression. I myself wouldn't mind and would be fine with 
making this the default, but I assume (plural) you wouldn't.

Comment at: clang/include/clang/Basic/LangOptions.def:163
 BENIGN_LANGOPT(CacheGeneratedPCH, 1, 0, "cache generated PCH files in memory")
+BENIGN_LANGOPT(PCHInstantiateTemplates, 1, 0, "performing pending template 
instantiations already while building a pch")
 COMPATIBLE_LANGOPT(ModulesDeclUse    , 1, 0, "require declaration of module 
rnk wrote:
> I think both sides of a PCH user and creator will need to agree on this, so 
> this isn't a "benign" langopt, is it? This is a regular one, and we should 
> diagnose mismatches between the PCH creation and use.
The flag needs to be used only when creating the PCH. Users don't care, they'll 
run the instantiation too, the flag will just make users do less repeated work.

Comment at: clang/lib/Sema/Sema.cpp:985-986
+    // FIXME: Instantiating implicit templates already in the PCH breaks some
+    // OpenMP-specific code paths, see https://reviews.llvm.org/D69585 .
+    if(LangOpts.PCHInstantiateTemplates && !LangOpts.OpenMP) {
rnk wrote:
> This really deserves to be debugged before we land it.
I debugged this more than 2 months ago, that's why the OpenMP code owner is 
added as a reviewer.  The initial diff (that I have no idea how to display 
here) included a suggested fix and the description said this was a WIP and 
included my findings about it. As far as I can tell the only effect that had 
was that this patch was sitting here all the time without a single reaction, so 
if nobody OpenMP related cares then I do not either.



cfe-commits mailing list

Reply via email to