1135535 - Fix MSVC compiler warnings in js/src and enable FAIL_ON_WARNINGS there

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

There are warnings like:
Warning: C4275 in js\src\jswrapper.h: non dll-interface class 'js::DirectProxyHandler' used as base for dll-interface class 'js::Wrapper'
js\src\jswrapper.h(58) : warning C4275: non dll-interface class 'js::DirectProxyHandler' used as base for dll-interface class 'js::Wrapper'
        obj-firefox\dist\include\js/Proxy.h(358) : see declaration of 'js::DirectProxyHandler'
        js\src\jswrapper.h(57) : see declaration of 'js::Wrapper'

It seems that Wrapper is marked JS_FRIEND_API which is always MOZ_EXPORT, while DirectProxyHandler is marked JS_PUBLIC_API which is not MOZ_EXPORT when STATIC_JS_API is defined. But there is no detailed description about the difference of these two macros. Should I mark DirectProxyHandler and its superclasses to JS_FRIEND_API as well?

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - C4805

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - C4067

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - C4258

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Hmmm, because of a bug in MSVC, warning C4200 "zero-sized array in struct/union" cannot be disabled from command line. There has been a bug report to Microsoft. [1] Before that problem gets fixed, I don't think we are able to completely fix/suppress all warnings here.

[1] https://connect.microsoft.com/VisualStudio/feedback/details/1114440/c4200-cant-be-disabled-from-the-command-line

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #1)
> ...snip...
> Should I mark DirectProxyHandler and its superclasses to JS_FRIEND_API as well?

That's probably reasonable, but I'll forward to efaust.

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 8567766 [details] [diff] [review]
patch 1 - C4805

Review of attachment 8567766 [details] [diff] [review]:
-----------------------------------------------------------------

In the past we've explicitly silenced this warning because it's a paint to explicitly cast to bool everywhere.  Can you do that instead?

Comment 8

[Eric Faust [:efaust]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #1)
> > ...snip...
> > Should I mark DirectProxyHandler and its superclasses to JS_FRIEND_API as well?
> 
> That's probably reasonable, but I'll forward to efaust.

Yes, that should be fine. I am surprised to hear that DirectProxyHandler is JS_PUBLIC_API. JS_FRIEND_API should be "more right" anyways.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Luke Wagner [:luke] from comment #7)
> Comment on attachment 8567766 [details] [diff] [review]
> patch 1 - C4805
> 
> Review of attachment 8567766 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In the past we've explicitly silenced this warning because it's a paint to
> explicitly cast to bool everywhere.  Can you do that instead?

OK, given the C++ spec says
"If the source type is bool, the value false is converted to zero and the value true is converted to one."
I guess this warning doesn't make much sense.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - Suppress C4805

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - C4275

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75e2dc05f76b

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7bf013c41cc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/196741bb1380
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ae7642fc71b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff4401748864

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7bf013c41cc1
https://hg.mozilla.org/mozilla-central/rev/196741bb1380
https://hg.mozilla.org/mozilla-central/rev/3ae7642fc71b
https://hg.mozilla.org/mozilla-central/rev/ff4401748864

Comment 15

[Chris Peterson [:cpeterson]]

MSVC compiler warnings have been fixed and bug 1230071 enabled warnings-as-errors for js/src.