Closed Bug 495156 Opened 11 years ago Closed 7 years ago

cleanup msvc warnings

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: gal, Assigned: gal)

References

(Blocks 1 open bug)

Details

(Whiteboard: [build_warning])

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
Attachment #380013 - Flags: review?
Comment on attachment 380013 [details] [diff] [review]
patch

> #define FUN_MINARGS(fun)     (((fun)->flags & JSFUN_FAST_NATIVE)              \
>-                              ? 0                                             \
>+                              ? ((uint16)0)                                   \

C++ style cast: uint16(0) -- and no parens around that!

> static bool
> evalCmp(LOpcode op, JSString* l, JSString* r)
> {
>     if (op == LIR_feq)
>-        return js_EqualStrings(l, r);
>+        return !!js_EqualStrings(l, r);

Isn't this a case whre we should change evalCmp to return JSBool (but continue to use true and false, of course)? Igor had a rationale for why.

>+        cond = !!js_EqualStrings(JSVAL_TO_STRING(l), JSVAL_TO_STRING(r));

This seems unfortunate too -- extra cost implied by conversion to bool, but js_EqualStrings can only ever return 0 or 1.

/be
Attachment #380013 - Flags: review? → review?(igor)
(In reply to comment #2)
> > static bool
> > evalCmp(LOpcode op, JSString* l, JSString* r)
> > {
> >     if (op == LIR_feq)
> >-        return js_EqualStrings(l, r);
> >+        return !!js_EqualStrings(l, r);
> 
> Isn't this a case whre we should change evalCmp to return JSBool (but continue
> to use true and false, of course)?

Ideally it is js_EqualStrings that should be modified to return bool. But that is a fast function which requires JSBool for now. It is possible to change  evalCmp, but then to avoid warnings (indicating an extra code to do the conversion) quite a few other bool variables would need to be turned into JSBool.

I guess leaving
(In reply to comment #3)
> I guess leaving

I meant leaving the warning could even serve as a good reminder to do MSVC-compatible bool suport, but having a bug on file (which is bug 479501) is way better then that.
As an alternative to adding !! in front of js_EqualString what about turning that into an inline that just calls the fast version and adding !!there? At least that would localize JSBool->bool conversion in one place.
Attachment #380013 - Flags: review?(igor)
Whiteboard: [build_warning]
FUN_MINARGS(fun) in jsfun.h was removed in Bug 581263.

jstracer.cpp was removed in https://hg.mozilla.org/mozilla-central/rev/60f879bef90e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.