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 uses") ---------------- 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. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69585/new/ https://reviews.llvm.org/D69585 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
