[PATCH] D69585: PerformPendingInstatiations() already in the PCH

[Luboš Luňák via Phabricator via cfe-commits]

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


================
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) {
----------------
dblaikie wrote:
> llunak wrote:
> > 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.
> > 
> > 
> So the original comment said "breaks some Open-MP specific code paths" - what kind of breakage occurs? (& I take it with the patch in its current form that breakage is present in the patch (as opposed to the previous version that conditionalized this feature so it didn't happen when OpenMP was enabled)?)
See https://reviews.llvm.org/D72759 . It's already been handled.



================
Comment at: clang/test/PCH/specialization-after-instantiation.cpp:2
+// Test this without pch.
+// RUN: %clang_cc1 -fsyntax-only -verify -DBODY %s
+
----------------
dblaikie wrote:
> Does this need testing without PCH here? Presumably that's already been tested elsewhere for some time now?
Technically not, but it can't hurt (it seems to be the common practice with testing PCHs), and I'm not aware of that elsewhere place (i.e. I've looked and couldn't find it).



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

https://reviews.llvm.org/D69585