Closed Bug 1313280 Opened 3 years ago Closed 3 years ago

Unsuppress MSVC warning C4819 and set the source character set instead

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: emk, Assigned: emk)

References

()

Details

Attachments

(6 files)

MSVC 2015 eventually allows to set the source character set.
Hm, local build didn't catch warnings somehow.
Comment on attachment 8805249 [details]
Bug 1313280 - Stop disabling MSVC warning C4819 and use the /utf-8 switch instead.

MozReview is very poor about syncing the review flag state between Bugzilla.
Attachment #8805249 - Flags: review?(mh+mozilla)
Attachment #8805523 - Flags: review?(jmathies)
Attachment #8805524 - Flags: review?(jyavenard)
Attachment #8805525 - Flags: review?(jmuizelaar)
Attachment #8805526 - Flags: review?(mh+mozilla)
Attachment #8805685 - Flags: review?(jorendorff)
Attachment #8805249 - Flags: review?(mh+mozilla)
Comment on attachment 8805524 [details]
Bug 1313280 - Suppress warnings from third-party sources.

https://reviewboard.mozilla.org/r/89286/#review88672

This is 3rd party code. will only complicate further resync.
Attachment #8805524 - Flags: review?(jyavenard) → review-
Comment on attachment 8805523 [details]
Bug 1313280 - Fix invalid non-UTF-8 bytes from widget/.

https://reviewboard.mozilla.org/r/89284/#review89246

There's a little whitespace right around here, you might as well grab that too since your editing whitespace here.
Attachment #8805523 - Flags: review?(jmathies) → review+
Comment on attachment 8805526 [details]
Bug 1313280 - Fix invalid non-UTF-8 bytes from Hunspell.

https://reviewboard.mozilla.org/r/89290/#review89420
Attachment #8805526 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8805524 [details]
Bug 1313280 - Suppress warnings from third-party sources.

https://reviewboard.mozilla.org/r/89286/#review89422

::: dom/media/platforms/omx/moz.build:54
(Diff revision 2)
>  if CONFIG['GNU_CXX']:
>      CXXFLAGS += ['-Wno-error=shadow']
> +
> +if CONFIG['_MSC_VER']:
> +    # Avoid warnings from third-party code that we can not modify.
> +    CXXFLAGS += ['-validate-charset-']

This flag is not supported by clang-cl (well, it is apparently supported on trunk, but we're using something older).
Attachment #8805524 - Flags: review?(mh+mozilla)
Comment on attachment 8805249 [details]
Bug 1313280 - Stop disabling MSVC warning C4819 and use the /utf-8 switch instead.

https://reviewboard.mozilla.org/r/89030/#review89424

::: old-configure.in:1047
(Diff revision 4)
>          WIN32_GUI_EXE_LDFLAGS=-SUBSYSTEM:WINDOWS,$WIN32_SUBSYSTEM_VERSION
>          DSO_LDOPTS=-SUBSYSTEM:WINDOWS,$WIN32_SUBSYSTEM_VERSION
>          _USE_CPP_INCLUDE_FLAG=1
>          _DEFINES_CFLAGS='-FI $(topobjdir)/mozilla-config.h -DMOZILLA_CLIENT'
>          _DEFINES_CXXFLAGS='-FI $(topobjdir)/mozilla-config.h -DMOZILLA_CLIENT'
> -        CFLAGS="$CFLAGS -W3 -Gy -Zc:inline"
> +        CFLAGS="$CFLAGS -utf-8 -W3 -Gy -Zc:inline"

Same as -validate-charset-, it's not supported by clang-cl.
Attachment #8805249 - Flags: review?(mh+mozilla)
Blocks: 1315152
:glandium, the review request status on MozReview stays "(r? cleared)" even though I pushed new changesets and resolved your comments. Is this a ReviewBoard bug? Or how can I set the review flag on ReviewBoard again?
See comment #27.
Flags: needinfo?(mh+mozilla)
Gently ping. This bug is now critical for non-Western developers due to bug 1315152.
Flags: needinfo?(jorendorff)
Flags: needinfo?(jmuizelaar)
Yeah, I don't know how to make mozreview set the r? flag again on its end. The important part is that the flag is on bugzilla.
Flags: needinfo?(mh+mozilla)
Comment on attachment 8805524 [details]
Bug 1313280 - Suppress warnings from third-party sources.

https://reviewboard.mozilla.org/r/89286/#review90274
Attachment #8805524 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8805249 [details]
Bug 1313280 - Stop disabling MSVC warning C4819 and use the /utf-8 switch instead.

https://reviewboard.mozilla.org/r/89030/#review90276

::: old-configure.in:1101
(Diff revision 5)
>          # that behavior) that it's better to turn it off.
> -        # MSVC warning C4819 warns some UTF-8 characters (e.g. copyright sign)
> -        # on non-Western system locales even if it is in a comment.
>          # MSVC warning wd4595 warns non-member operator new or delete functions
>          # may not be declared inline, as of VS2015 Update 2.
> -        CFLAGS="$CFLAGS -wd4244 -wd4267 -wd4819"
> +        CFLAGS="$CFLAGS -wd4244 -wd4267"

Mmmm it might be better to keep the flag when building with clang-cl.

Please also file a followup bug to change the unconditional clang-cl exclusion into a feature check (whether the compiler supports -utf-8/-validate-charset-)
Attachment #8805249 - Flags: review?(mh+mozilla)
Comment on attachment 8805525 [details]
Bug 1313280 - Fix invalid non-UTF-8 bytes from cairo

https://reviewboard.mozilla.org/r/89288/#review90376
Attachment #8805525 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8805249 [details]
Bug 1313280 - Stop disabling MSVC warning C4819 and use the /utf-8 switch instead.

https://reviewboard.mozilla.org/r/89030/#review90276

> Mmmm it might be better to keep the flag when building with clang-cl.
> 
> Please also file a followup bug to change the unconditional clang-cl exclusion into a feature check (whether the compiler supports -utf-8/-validate-charset-)

> Mmmm it might be better to keep the flag when building with clang-cl.

Clang always uses utf-8 as the source character set. So we don't have to suppress warnings even if clang-cl does not recognize /utf-8 yet. Moreover, clang-cl does not really support the /wd4819 option. It will just ignore /wdxxxx options.

> Please also file a followup bug to change the unconditional clang-cl exclusion into a feature check (whether the compiler supports -utf-8/-validate-charset-)

I unconditionally used -Wno-invalid-source-encoding instead. (Unlike /wdxxxx, it will actually suppress clang-cl warnings. /validate-charset- will just map to this option[1].)

[1] https://reviews.llvm.org/D23945
(In reply to Masatoshi Kimura [:emk] from comment #40)
> I unconditionally used -Wno-invalid-source-encoding instead. (Unlike
> /wdxxxx, it will actually suppress clang-cl warnings. /validate-charset-
> will just map to this option[1].)

I don't see this in your patch.
Flags: needinfo?(VYV03354)
(In reply to Mike Hommey [:glandium] from comment #41)
> (In reply to Masatoshi Kimura [:emk] from comment #40)
> > I unconditionally used -Wno-invalid-source-encoding instead. (Unlike
> > /wdxxxx, it will actually suppress clang-cl warnings. /validate-charset-
> > will just map to this option[1].)
> 
> I don't see this in your patch.

MozReview didn't re-flag the review request for some reason.
https://reviewboard.mozilla.org/r/89286/diff/4#index_header
Flags: needinfo?(VYV03354)
Attachment #8805685 - Flags: review?(jorendorff) → review?(jdemooij)
Looks like :jorendorff is busy.
Flags: needinfo?(jorendorff)
Flags: needinfo?(jmuizelaar)
Comment on attachment 8805685 [details]
Bug 1313280 - Fix invalid non-UTF-8 bytes from js/

https://reviewboard.mozilla.org/r/89400/#review91224

Good catch.
Attachment #8805685 - Flags: review?(jdemooij) → review+
Comment on attachment 8805524 [details]
Bug 1313280 - Suppress warnings from third-party sources.

https://reviewboard.mozilla.org/r/89286/#review91432
Comment on attachment 8805249 [details]
Bug 1313280 - Stop disabling MSVC warning C4819 and use the /utf-8 switch instead.

https://reviewboard.mozilla.org/r/89030/#review91434
Attachment #8805249 - Flags: review?(mh+mozilla) → review+
> There was an error executing the Autoland request on autoland
> Requested by emk hg error in cmd: hg rewritecommitdescriptions --descriptions=/tmp/tmpuX3sLH
> 45b6190dd05f6e7b92f7a24b9062faa04b38631c: saved backup bundle to
> /repos/mozreview-gecko/.hg/strip-backup/f63022a4b43b-30934d4c-replacing.hg warning: ignoring
> unknown working parent 236f06dcb09d! rev: fe9ceb61b8d3 -> fe9ceb61b8d3 rev: 7c1999e12414 ->
> 1cd1fbaf6b95 rev: f63022a4b43b -> 425bf84855df abort: unknown revision
> 'cec9751f6b5a39da579347e7b1970f84df6715a1'!

Recently MozReview has never worked for me :(
I'll try manual landing later.
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53b841d7ad4d
Fix invalid non-UTF-8 bytes from widget/. r=jimm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a37db7ce8715
Fix invalid non-UTF-8 bytes from cairo. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/8abca49e8f5e
Fix invalid non-UTF-8 bytes from Hunspell. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/7fec84d7667e
Fix invalid non-UTF-8 bytes from js/. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ede191f1740
Suppress warnings from third-party sources. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb8d5979d0f1
Stop disabling MSVC warning C4819 and use the /utf-8 switch instead. r=glandium
Assignee: nobody → VYV03354
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.