Closed Bug 1415470 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, enhancement, P3)

Product:
Firefox Build System
Component:
General
Platform:
Unspecified
Windows
Type:
enhancement

Tracking

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

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1415470 - build: Enable VS2017 C5038 initializer list order warnings (like gcc -Wreorder).
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1415470 - a11y: Fix VS2017 initializer list order warnings.
59 bytes, text/x-review-board-request
yzen
: review+
Details
Bug 1415470 - build: Remove always-true version check for VS >= 2015.
59 bytes, text/x-review-board-request
glandium
: review+
Details 
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 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 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)
(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.
(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 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 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+
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
(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
Won't fix for 58. Let it ride the train.
status-firefox58: affected → wontfix
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: