Page MenuHomePhabricator

[MS] Warn when shadowing template parameters under -fms-compatibility
ClosedPublic

Authored by rnk on Sep 11 2019, 1:08 PM.

Details

Summary

C++ does not allow shadowing template parameters, but previously we
allowed it under -fms-extensions. Now this behavior is controlled by
-fms-compatibility, and we emit a -Wmicrosoft-template warning when it
happens.

Fixes PR43265

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Sep 11 2019, 1:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 11 2019, 1:08 PM
thakis accepted this revision.Sep 11 2019, 8:00 PM

Nice!

Hopefully this is rare enough that putting it under an existing warning flag won't be too inconvenient.

This revision is now accepted and ready to land.Sep 11 2019, 8:00 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 11:26 AM
aganea added a subscriber: aganea.Nov 29 2019, 2:32 PM

You probably already know this, but the behavior between MSVC & Clang is different as to the value of the constant (testcase for PR43265).
See: https://godbolt.org/z/y4uvgp
Clang gives 0, while MSVC gives 5. This incompatibility is a potential source of failure, one of our tests broke because of that.

Do you think this patch could/should be back-ported into 9.0.1? At least having the warning gives an indication about a potential failure.

aganea added a subscriber: saudi.Nov 29 2019, 2:32 PM
rnk added a comment.Dec 3 2019, 2:19 PM

@hans, are we still accepting 9.0.1 patches? I thought we'd already made a release candidate.

hans added a subscriber: tstellar.Dec 4 2019, 1:30 AM
In D67463#1767919, @rnk wrote:

@hans, are we still accepting 9.0.1 patches? I thought we'd already made a release candidate.

+Tom: ^

In D67463#1767919, @rnk wrote:

@hans, are we still accepting 9.0.1 patches? I thought we'd already made a release candidate.

I'm still accepting patches until rc2 (which should have been Monday, but was delayed), so I can pull it if you think it is important.

rnk added a comment.Dec 4 2019, 12:52 PM
In D67463#1767919, @rnk wrote:

@hans, are we still accepting 9.0.1 patches? I thought we'd already made a release candidate.

I'm still accepting patches until rc2 (which should have been Monday, but was delayed), so I can pull it if you think it is important.

You know, it's a significant change in behavior. This will cause hard errors for users of -fms-extensions who do not use -fms-compatibility. Maybe it's better to release that changes in the 10.0 release.

In D67463#1769557, @rnk wrote:
In D67463#1767919, @rnk wrote:

@hans, are we still accepting 9.0.1 patches? I thought we'd already made a release candidate.

I'm still accepting patches until rc2 (which should have been Monday, but was delayed), so I can pull it if you think it is important.

You know, it's a significant change in behavior. This will cause hard errors for users of -fms-extensions who do not use -fms-compatibility. Maybe it's better to release that changes in the 10.0 release.

Ok, we'll leave it out.