Enable VS2017 C5038 initializer list order warnings (like gcc -Wreorder)

RESOLVED FIXED in Firefox 59

Status

()

Core
Build Config
P3
normal
RESOLVED FIXED
14 days ago
6 hours ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 wontfix, firefox59 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

14 days ago
C5038 is a new warning in VS2017, similar to gcc and clang's -Wreorder, which is enabled by -Wall. We should enable C5038 so Windows developers can see these warnings locally instead of when gcc and clang fail with warnings-as-errors on Try.

https://blogs.msdn.microsoft.com/vcblog/2017/07/21/diagnostic-improvements-in-vs2017-15-3-0/

Here is a green Try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4a74ca255f544cf83d31eee2d19ddbc721d4c45
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

14 days ago
mozreview-review
Comment on attachment 8926303 [details]
Bug 1415470 - a11y: Fix VS2017 initializer list order warnings.

https://reviewboard.mozilla.org/r/197580/#review202954

Thanks
Attachment #8926303 - Flags: review?(yzenevich) → review+

Comment 4

13 days ago
mozreview-review
Comment on attachment 8926302 [details]
Bug 1415470 - build: Enable VS2017 C5038 initializer list order warnings (like gcc -Wreorder).

https://reviewboard.mozilla.org/r/197578/#review203180

::: js/src/old-configure.in:780
(Diff revision 1)
>              # which clang-cl doesn't understand.
>              CXXFLAGS="$CXXFLAGS -Wno-ignored-attributes"
>          fi
> +
> +        # Enable extra warnings
> +        CXXFLAGS="$CXXFLAGS -w15038" # C5038 initializer list order warnings

Don't you want it as -We?

You should only add the flag on msvc 2017. We still allow to build with 2015 (which I actually don't know whether that's an oversight or intentional, but it's the current state).
Attachment #8926302 - Flags: review?(mh+mozilla)
(Assignee)

Comment 5

13 days ago
(In reply to Mike Hommey [:glandium] from comment #4)
> > +        CXXFLAGS="$CXXFLAGS -w15038" # C5038 initializer list order warnings
> 
> Don't you want it as -We?

Like -Werror, -we will enable this warning as an error even in directories that AllowCompilerWarnings(). -w1 will just enable the warning and let it be treated as a warning or error as appropriate for each directory.

> You should only add the flag on msvc 2017. We still allow to build with 2015
> (which I actually don't know whether that's an oversight or intentional, but
> it's the current state).

OK. I'll add a compiler version check.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

11 days ago
(In reply to Mike Hommey [:glandium] from comment #4)
> You should only add the flag on msvc 2017. We still allow to build with 2015
> (which I actually don't know whether that's an oversight or intentional, but
> it's the current state).

I added a version check for VS2017+ in old-configure.in. I don't think a version check is necessary for disabling the warnings (with -wd5038) in moz.build. I don't have VS2015 to test, but disabling an unrecognized (bogus) warning number in VS2017 is not an error.

Comment 12

8 days ago
mozreview-review
Comment on attachment 8926302 [details]
Bug 1415470 - build: Enable VS2017 C5038 initializer list order warnings (like gcc -Wreorder).

https://reviewboard.mozilla.org/r/197578/#review204380

::: old-configure.in:197
(Diff revision 3)
>              AC_DEFINE(MSVC_HAS_DIA_SDK)
>          fi
>  
> +        if test "$_MSC_VER" -ge "1910"; then # VS2017+
> +            # C5038: initializer list order warnings
> +            CXXFLAGS="$CXXFLAGS -w15038"

maybe add a comment that it *is* intended to be -w1nnnn, because I'm afraid someone else comes later and thinks "hey there's a typo on the warning number".
Attachment #8926302 - Flags: review?(mh+mozilla) → review+

Comment 13

8 days ago
mozreview-review
Comment on attachment 8927535 [details]
Bug 1415470 - build: Remove always-true version check for VS >= 2015.

https://reviewboard.mozilla.org/r/198838/#review204382
Attachment #8927535 - Flags: review?(mh+mozilla) → review+

Comment 14

7 days ago
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cec4f6e0be00
a11y: Fix VS2017 initializer list order warnings. r=yzen
https://hg.mozilla.org/integration/mozilla-inbound/rev/819ad76c6377
build: Remove always-true version check for VS >= 2015. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e35281e0eaf
build: Enable VS2017 C5038 initializer list order warnings (like gcc -Wreorder). r=glandium
(Assignee)

Comment 15

7 days ago
(In reply to Mike Hommey [:glandium] from comment #12)
> maybe add a comment that it *is* intended to be -w1nnnn, because I'm afraid
> someone else comes later and thinks "hey there's a typo on the warning
> number".

I added the following comment in old-configure.in:

# The -w1#### flag treats warning C#### as if it was a warning level
# 1 warning, and thus enables it because we enable /W3 warnings. We
# don't use -we#### because it would enable warning C#### but treat
# it as an error, even in third-party code.
# https://docs.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level
https://hg.mozilla.org/mozilla-central/rev/cec4f6e0be00
https://hg.mozilla.org/mozilla-central/rev/819ad76c6377
https://hg.mozilla.org/mozilla-central/rev/7e35281e0eaf
Status: NEW → RESOLVED
Last Resolved: 6 days ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Won't fix for 58. Let it ride the train.
status-firefox58: affected → wontfix
You need to log in before you can comment on or make changes to this bug.