[PATCH] MS compatibility flag implies delayed parsing

There was a small discussion on IRC about the
-fdelayed-template-parsing flag always going hand-in-hand with the
-fms-compatibility flag. Since the implication is already there (you
don't really have ms compatibility without the delayed template
parsing, to a certain extent), this patch makes it more explicit. So
when you pass the fms-compatibility flag, it automatically enables the
fdelayed-template-parsing flag as well. If the user really does want
one without the other, they can always pass -fms-compatibility
-fnodelayed-template-parsing.

~Aaron

mscompat.diff (2.62 KB)

Concept is fine; the patch doesn't actually implement what you say it
does, though. Specifically, -fms-compatibility
-fnodelayed-template-parsing won't work with this patch.

-Eli

Is there a better example I should follow instead?

~Aaron

Here is a second attempt at the patch -- this time it works the way I
wanted it to work! Thanks for the review.

~Aaron

mscompat.diff (3.61 KB)

I don't see why the changes to lib/Frontend/CompilerInvocation.cpp and
include/clang/Driver/CC1Options.td are necessary. The changes to
lib/Driver/Tools.cpp look fine.

-Eli

How about we remove --fdelayed-template-parsing altogether and check
for fms-compatibility to do late template parsing?

Anybody else needs delayed template parsing without microsoft compatibility?

I don't see why the changes to lib/Frontend/CompilerInvocation.cpp and
include/clang/Driver/CC1Options.td are necessary. The changes to
lib/Driver/Tools.cpp look fine.

I'm not certain I understand why the two are separate from one
another. Can you give me a quite education on the topic?

How about we remove --fdelayed-template-parsing altogether and check
for fms-compatibility to do late template parsing?

Anybody else needs delayed template parsing without microsoft compatibility?

That would be the best solution IMO, but I wasn't sure if anyone was
relying on them being separated. If no one objects, I can remove the
delayed template parsing param and simply roll the functionality into
MS compatibility. But I do have one question on this -- should I
simply remove the flag entirely (so people start getting warnings
about unused flags), or should I leave the flag in and make it no-op?

Thanks!

~Aaron

As far as I remember it was Doug indicating that it could be beneficial to have it separately, as I think Francois had demonstrated that only parsing on-demand (at the end of the TU) improved performance somewhat.

– Matthieu

I suggest removing the driver-level "-fdelayed-template-parsing" (from lib/Driver/Tools.cpp and include/clang/Driver/Options.td). However, please leave -fdelayed-template-parsing as a separate "-cc1" option, because delayed template parsing can be useful even when we're not emulating MSVC.

  - Doug

I've attached a patch that implements these changes. I did still
clean up CompilerInvocation to remove a duplicate assignment.

~Aaron

mscompat.diff (2.54 KB)

Looks great, please go ahead and commit!

  - Doug

This doesn't work.
Some tests depend on -fno-delayed-template-parsing to exist at the driver level.

Good catch! I've revised the patch so that
-fno-delayed-template-parsing will be honored in the driver.

~Aaron

mscompat.diff (2.73 KB)

Having option -fno-delayed-template-parsing without
-fdelayed-template-parsing doesn't sound right to me.
How about we keep it.

Shouldn't those tests that are currently using
-fno-delayed-template-parsing be modified to use -fms-compatibility or
pass -fno-delayed-template-parsing via -Xclang? Or are those test case
enough to justify the existence of -fno-delayed-template-parsing as a
driver-visible flag?

- David

This doesn't work.
Some tests depend on -fno-delayed-template-parsing to exist at the driver level.

Good catch! I've revised the patch so that
-fno-delayed-template-parsing will be honored in the driver.

Having option -fno-delayed-template-parsing without
-fdelayed-template-parsing doesn't sound right to me.
How about we keep it.

Are you suggesting we keep the flag, but still make it implicitly on
when ms-compatibility is specified? Or just keep things as they are
today? I am fine with keeping the flag, but I still think we should
implicitly turn it on for ms-compatibility.

Shouldn't those tests that are currently using
-fno-delayed-template-parsing be modified to use -fms-compatibility or
pass -fno-delayed-template-parsing via -Xclang? Or are those test case
enough to justify the existence of -fno-delayed-template-parsing as a
driver-visible flag?

I usually figure that if there's a test for it, there's a reason for
it, so I think they do justify the ability to turn off delayed
template parsing explicitly.

~Aaron

My best guess, glancing at the tests, is that the switch was used to
make the tests portable - probably just because the switch existed and
was the smallest change necessary to get the tests to pass across
platforms. It doesn't look like there would be a problem with just
-fno-ms-compatibility being used entirely for those tests but I'm
open to suggestions.

In fact they were pretty much all added in r138942 by Francois:

"Enable -fdelayed-template-parsing by default on Win32.
I had to force -fno-delayed-template-parsing on some Index tests
because delayed template parsing will change the output of some
tests."

So presumably if we make -fms-compatibility default on in Win32 (which
if we haven't already, we probably should if it's going to subsume
-fdelayed-template-parsing) then it make sense to just change these
all to -fno-ms-compatibility.

- David

This doesn’t work.
Some tests depend on -fno-delayed-template-parsing to exist at the driver level.

Good catch! I’ve revised the patch so that
-fno-delayed-template-parsing will be honored in the driver.

Having option -fno-delayed-template-parsing without
-fdelayed-template-parsing doesn’t sound right to me.
How about we keep it.

Are you suggesting we keep the flag, but still make it implicitly on
when ms-compatibility is specified? Or just keep things as they are
today? I am fine with keeping the flag, but I still think we should
implicitly turn it on for ms-compatibility.

Shouldn’t those tests that are currently using
-fno-delayed-template-parsing be modified to use -fms-compatibility or
pass -fno-delayed-template-parsing via -Xclang? Or are those test case
enough to justify the existence of -fno-delayed-template-parsing as a
driver-visible flag?

I usually figure that if there’s a test for it, there’s a reason for
it, so I think they do justify the ability to turn off delayed
template parsing explicitly.

My best guess, glancing at the tests, is that the switch was used to
make the tests portable - probably just because the switch existed and
was the smallest change necessary to get the tests to pass across
platforms. It doesn’t look like there would be a problem with just
-fno-ms-compatibility being used entirely for those tests but I’m
open to suggestions.

In fact they were pretty much all added in r138942 by Francois:

“Enable -fdelayed-template-parsing by default on Win32.
I had to force -fno-delayed-template-parsing on some Index tests
because delayed template parsing will change the output of some
tests.”

So presumably if we make -fms-compatibility default on in Win32 (which
if we haven’t already, we probably should if it’s going to subsume
-fdelayed-template-parsing) then it make sense to just change these
all to -fno-ms-compatibility.

Hold your horses! Win32 != borked MSVC headers! There’s also GCC for windows, and as I don’t know what implications the delayed template parsing has on the libstdc++/libc++ headers, it’s not a good idea to make this a default for Win32.

Ruben

Perhaps you misunderstood - delayed template parsing already is the
default in windows & has been for a couple of months.

- David

We're talking about -fms-compatibility, which does = borked MSVC
headers, which generally require delayed template parsing. That's why
the discussion is happening about making -fms-compatibility
automatically adding -fdelayed-template-parsing.

~Aaron