Closed Bug 1135535 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: xidorn, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 1 obsolete file)

No description provided.
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?
Flags: needinfo?(jdemooij)
Keywords: leave-open
Attached patch patch 1 - C4805 (obsolete) — Splinter Review
Attachment #8567766 - Flags: review?(luke)
Attached patch patch 2 - C4067Splinter Review
Attachment #8567771 - Flags: review?(luke)
Attached patch patch 3 - C4258Splinter Review
Attachment #8567773 - Flags: review?(luke)
Assignee: nobody → quanxunzhen
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
(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.
Flags: needinfo?(jdemooij) → needinfo?(efaustbmo)
Attachment #8567771 - Flags: review?(luke) → review+
Attachment #8567773 - Flags: review?(luke) → review+
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?
Attachment #8567766 - Flags: review?(luke)
(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.
Flags: needinfo?(efaustbmo)
(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.
Attachment #8567766 - Attachment is obsolete: true
Attachment #8568380 - Flags: review?(luke)
Attached patch patch 4 - C4275Splinter Review
Attachment #8568383 - Flags: review?(luke)
Attachment #8568380 - Flags: review?(luke) → review+
Attachment #8568383 - Flags: review?(luke) → review+
MSVC compiler warnings have been fixed and bug 1230071 enabled warnings-as-errors for js/src.
Status: NEW → RESOLVED
Closed: 4 years ago
Depends on: 1230071
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.