[PATCH] D69585: Add option to instantiate templates already in the PCH

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

llunak marked an inline comment as done.
llunak added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:5610
+  if (Args.hasFlag(options::OPT_fpch_instantiate_templates,
+                   options::OPT_fno_pch_instantiate_templates, false))
+    CmdArgs.push_back("-fpch-instantiate-templates");
----------------
rnk wrote:
> llunak wrote:
> > rnk wrote:
> > > Does MSVC default to this behavior? Should this default to true with clang-cl /Yu / /Yc? This can be future work and does not need to be part of this patch.
> > Since MSVC is noticeably faster for an equivalent PCH compile than current Clang, presumably it instantiates templates already in the PCH. But that doesn't really matter for this patch, if it were ok to enable this by default for clang-cl, than it would be ok also for clang itself. That cannot be done now though, https://reviews.llvm.org/D69585#1946765 points out a corner case where this change makes a valid compilation error out, and that's the reason for having this behind a flag. I expect Clang could possibly be adjusted to bail out and delay template instantiantion in such a case until it can be performed successfully, but given the response rate to my PCH patches I first wanted to get the feature in somehow, and I can try to make the flag default/irrelevant later.
> > 
> Right, I guess what I mean is, for that example where instantiation at the end of the PCH creates an error, does MSVC emit an error? I just checked, and it looks like the answer is yes, so it seems like we could make this the default behavior for `clang-cl /Yc` without much further discussion.
That's a good point. Yes, since MSVC creates PCHs by basically compiling an empty .cpp, it apparently does instantiate templates as a side-effect of that, and https://reviews.llvm.org/D69585#1946765 indeed doesn't work with MSVC. So no harm in enabling the option for clang-cl.

I'll update the patch.



Repository:
  rC Clang

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

https://reviews.llvm.org/D69585