Closed
Bug 1135535
Opened 9 years ago
Closed 9 years ago
Fix MSVC compiler warnings in js/src and enable FAIL_ON_WARNINGS there
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: xidorn, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 1 obsolete file)
1.96 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
2.73 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
2.24 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
2.43 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8567766 -
Flags: review?(luke)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8567771 -
Flags: review?(luke)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8567773 -
Flags: review?(luke)
Updated•9 years ago
|
Assignee: nobody → quanxunzhen
Assignee | ||
Comment 5•9 years ago
|
||
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•9 years ago
|
||
(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)
Updated•9 years ago
|
Attachment #8567771 -
Flags: review?(luke) → review+
Updated•9 years ago
|
Attachment #8567773 -
Flags: review?(luke) → review+
Comment 7•9 years ago
|
||
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)
Comment 8•9 years ago
|
||
(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)
Assignee | ||
Comment 9•9 years ago
|
||
(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.
Assignee | ||
Comment 10•9 years ago
|
||
Attachment #8567766 -
Attachment is obsolete: true
Attachment #8568380 -
Flags: review?(luke)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8568383 -
Flags: review?(luke)
Updated•9 years ago
|
Attachment #8568380 -
Flags: review?(luke) → review+
Updated•9 years ago
|
Attachment #8568383 -
Flags: review?(luke) → review+
Assignee | ||
Comment 12•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75e2dc05f76b
Assignee | ||
Comment 13•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
MSVC compiler warnings have been fixed and bug 1230071 enabled warnings-as-errors for js/src.
Status: NEW → RESOLVED
Closed: 9 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.
Description
•