495156 - cleanup msvc warnings

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Brendan Eich [:brendan]]

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

Comment 3

[Igor Bukanov]

(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

Comment 4

[Igor Bukanov]

(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.

Comment 5

[Igor Bukanov]

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.

Comment 6

[:Cykesiopka]

FUN_MINARGS(fun) in jsfun.h was removed in Bug 581263.

jstracer.cpp was removed in https://hg.mozilla.org/mozilla-central/rev/60f879bef90e