Closed Bug 603825 Opened 14 years ago Closed 14 years ago

Fix a bunch of warnings

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

GCC on Linux:

../jsbool.cpp: In function ‘JSBool bool_toString(JSContext*, uintN, js::Value*)’:
../jsbool.cpp:98: warning: ‘b’ may be used uninitialized in this function
../jsbool.cpp: In function ‘JSBool bool_toSource(JSContext*, uintN, js::Value*)’:
../jsbool.cpp:81: warning: ‘b’ may be used uninitialized in this function
../jsbool.cpp: In function ‘JSBool bool_valueOf(JSContext*, uintN, js::Value*)’:../jsbool.cpp:113: w
arning: ‘b’ may be used uninitialized in this function

../jsstr.cpp: In function ‘JSBool js_str_toString(JSContext*, uintN, js::Value*)’:
../jsstr.cpp:951: warning: ‘str’ may be used uninitialized in this function
../jsstr.cpp: In function ‘JSBool str_toSource(JSContext*, uintN, js::Value*)’:
../jsstr.cpp:908: warning: ‘str’ may be used uninitialized in this function

../methodjit/Compiler.cpp: In member function ‘bool js::mjit::Compiler::jsop_callprop_str(JSAtom*)’:
../methodjit/Compiler.cpp:2821: warning: unused variable ‘funFe’


All of these except the last two were introduced by bug 514570.  Here's the jsbool.cpp code:

    bool b;
    if (!GetPrimitiveThis(cx, vp, &b))
        return false;

    JSAtom *atom = cx->runtime->atomState.booleanAtoms[b ? 1 : 0];

It seems that GCC is reasonably smart about detecting the uninitialized variables - if GetPrimitiveThis() returns 'false' when it doesn't initialize 'b', then GCC doesn't give the warning.  However, GetPrimitiveThis() instead returns 'ReportIncompatibleMethod(cx, vp, Behavior::getClass());' which is always false.  It's not smart enough to detect that.  So I changed ReportIncompatibleMethod() to have a return void type and make the 'return false' explicit in GetPrimitiveThis.  This removed all the jsbool.cpp and jsstr.cpp warnings and seemed preferable to initializing several variables unnecessarily.

(I've always found error-reporting functions that always return false a bit creepy, and now I finally have some concrete evidence why they shouldn't be used :)

The methodjit/Compiler.cpp fix was trivial.
Attachment #482728 - Flags: review?(jwalden+bmo)
Comment on attachment 482728 [details] [diff] [review]
patch (against TM 55344:db5b6e24f421)

That's unfortunate.  I've always been a fan of report-and-return-false functions for the slightly briefer code needed to use them; further, they can unify errors returned in multiple places into a single call.  (See js_SetPropertyHelper, possibly in past revisions, for such a case.)  But if the compiler's tripping over them here, and there's no attribute to add to tell it not to trip (online gcc docs indicate nothing), I guess this is the right thing to do.
Attachment #482728 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/tracemonkey/rev/8212d565d2a4
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/8212d565d2a4
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.