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