Closed Bug 549010 Opened 14 years ago Closed 14 years ago

compiler warnings Tracemonkey tip and MSVC

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
All
Windows XP
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: MikeM, Assigned: MikeM)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 10 obsolete files)

patch without jsopcode.h changes
144.09 KB, patch
igor
: review+
Details | Diff | Splinter Review
Attached patch 1st attempt at warnings fix. (obsolete) — — Splinter Review 
Just knocking out a slug of MSVC compiler warnings. The header related warnings drive you absolutely nuts as they repeat over and over the build output...

BTW Thanks to David who fixed the last round a warnings I reported months back!
David can you review and land this?  I can't, no privledges...

CC'ing those who may care...
Hopefully my patch is up to snuff.

Thanks!

Here's the list of warnings that I fixed:
------------------------------------------------
js\src\jsobj.h(365) : warning C4800: 'jsval *const ' : forcing value to bool 'true' or 'false' (performance warning)
js\src\jscntxt.h(286) : warning C4800: 'JSStackFrame *const ' : forcing value to bool 'true' or 'false' (performance warning)
src\jscntxt.h(333) : warning C4099: 'js::TraceMonitor' : type name first seen using 'class' now seen using 'struct'
src\jsscope.h(556) : warning C4099: 'JSScope' : type name first seen using 'struct' now seen using 'class'
src\jscntxt.h(1263) : warning C4355: 'this' : used in base member initializer list
src\jstypedarray.cpp(404) : warning C4244: '=' : conversion from 'int32' to 'uint8', possible loss of data
src\jstypedarray.cpp(1340) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstypedarray.cpp(1343) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstypedarray.cpp(1346) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstypedarray.cpp(1349) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstypedarray.cpp(1352) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstypedarray.cpp(1355) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstypedarray.cpp(1358) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstypedarray.cpp(1361) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstypedarray.cpp(1364) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jsscope.h(605) : warning C4242: 'initializing' : conversion from 'uintN' to 'uint8', possible loss of data
src\jstracer.h(375) : warning C4099: 'js::TypeMap' : type name first seen using 'struct' now seen using 'class'
src\jsscript.cpp(1619) : warning C4242: '=' : conversion from 'uintN' to 'uint16', possible loss of data
src\jsscan.cpp(653) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jsscope.h(605) : warning C4242: 'initializing' : conversion from 'uintN' to 'uint8', possible loss of data
src\jsscope.h(605) : warning C4242: 'initializing' : conversion from 'uintN' to 'uint8', possible loss of data
src\jsscope.h(605) : warning C4242: 'initializing' : conversion from 'intN' to 'int16', possible loss of data
src\json.cpp(372) : warning C4805: '&=' : unsafe mix of type 'bool' and type 'JSBool' in operation
src\json.cpp(372) : warning C4805: '&' : unsafe mix of type 'bool' and type 'JSBool' in operation
src\json.cpp(372) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jsscopeinlines.h(133) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jsobj.cpp(6811) : warning C4800: 'const JSNative' : forcing value to bool 'true' or 'false' (performance warning)
src\jsapi.cpp(5598) : warning C4242: '=' : conversion from 'JSBool' to 'JSPackedBool', possible loss of data
src\jscntxt.cpp(1656) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jsdbgapi.cpp(1466) : warning C4800: 'JSPackedBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstracer.cpp(4515) : warning C4265: 'js::SlotMap' : class has virtual functions, but destructor is not virtual instances of this class may not be destructed correctly
src\jstracer.cpp(4531) : warning C4265: 'js::DefaultSlotMap' : class has virtual functions, but destructor is not virtual instances of this class may not be destructed correctly
src\jstracer.cpp(7802) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstracer.cpp(13762) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)
src\jstracer.cpp(2786) : warning C4800: 'JSBool' : forcing value to bool 'true' or 'false' (performance warning)

--------------------------------------------------------------------------------
Here are the 13 remaining warnings from jstracer.cpp which I did NOT fix.
The "behavior change" ones should probably be looked at by somebody smarter than me.

1>.\jstracer.cpp(3552) : warning C4242: 'argument' : conversion from 'unsigned int' to 'uint16', possible loss of data
1>.\jstracer.cpp(3558) : warning C4242: 'argument' : conversion from 'unsigned int' to 'uint16', possible loss of data
1>.\jstracer.cpp(3680) : warning C4242: 'argument' : conversion from 'unsigned int' to 'uint16', possible loss of data
1>.\jstracer.cpp(3867) : warning C4242: 'argument' : conversion from 'ptrdiff_t' to 'uint16', possible loss of data
1>.\jstracer.cpp(4084) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
1>.\jstracer.cpp(4422) : warning C4242: 'argument' : conversion from 'ptrdiff_t' to 'uint16', possible loss of data
1>.\jstracer.cpp(5012) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
1>.\jstracer.cpp(7962) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
1>.\jstracer.cpp(8382) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
1>.\jstracer.cpp(10524) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
1>.\jstracer.cpp(10912) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
1>.\jstracer.cpp(12472) : warning C4242: 'argument' : conversion from 'uintN' to 'uint16', possible loss of data
1>c:\dev\common\spidermonkey\js\src\jsrecursion.cpp(239) : warning C4242: 'argument' : conversion from 'uintN' to 'uint16', possible loss of data
Attachment #429265 - Flags: review?(dmandelin)
Hey Mike. Thanks for going through the warnings. Few people on the JS team build with msvc so we sometimes miss them. The house style to deal with them is "!!a"  I believe (instead of != JS_FALSE). CCing the style police.
Thanks Gal...are an outer set of parenthesis desired? or leave them out?
Attached patch Patch #2 (obsolete) — — Splinter Review 
fixed suggestions made by Gal. Fixes a few more warnings.
Assignee: general → MikeM
Attachment #429265 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #429265 - Flags: review?(dmandelin)
Latest set of compile warnings fixed. New patch.
Here are the remaining compile warnings from jstracer.cpp:

Should I just ignore the POD type warnings?  I think it would be nice if that could be fixed the right way since some compilers will may not initialize those structs like VC++ does.
------------------------------------
jstracer.cpp(3867) : warning C4242: 'argument' : conversion from 'ptrdiff_t' to 'uint16', possible loss of data
jstracer.cpp(4084) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
jstracer.cpp(4422) : warning C4242: 'argument' : conversion from 'ptrdiff_t' to 'uint16', possible loss of data
jstracer.cpp(5012) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized jstracer.cpp(7962) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
jstracer.cpp(8382) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
jstracer.cpp(10524) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
jstracer.cpp(10912) : warning C4345: behavior change: an object of POD type constructed with an initializer of the form () will be default-initialized
----------------------------

Also I've got some new downstream warnings now with the latest JS code:
--------------------------------------------------------------------
--------
js\src\jsopcode.h(388) : warning C4100: 'op' : unreferenced formal parameter

Source: 
js_GetStackDefs(JSContext *cx, const JSCodeSpec *cs, JSOp op, JSScript *script,jsbytecode *pc)

(JSOp op) appears to only we used in this function for DEBUG builds.
Can I just remove it completely?

-------

jsobj.h(191) : warning C4512: 'JSObjectMap' : assignment operator could not be generated  jsobj.h(184) : see declaration of 'JSObjectMap'

Need some insight on this one....

-------
src\jsinterp.h(191) : warning C4100: 'depth' : unreferenced formal parameter

Source:
void
JSStackFrame::assertValidStackDepth(uintN depth)
{
    JS_ASSERT(0 <= regs->sp - StackBase(this));
    JS_ASSERT(depth <= uintptr_t(regs->sp - StackBase(this)));
}

Can I just remove all references to this function during release builds?
Why incur the cost to call to a function that does nothing???
-------

jsarray.h(93) : warning C4100: 'cx' : unreferenced formal parameter

static JS_INLINE JSObject *
js_GetProtoIfDenseArray(JSContext *cx, JSObject *obj)
{
    return OBJ_IS_DENSE_ARRAY(cx, obj) ? OBJ_GET_PROTO(cx, obj) : obj;
}
#define OBJ_IS_DENSE_ARRAY(cx,obj)  (obj)->isDenseArray()
#define OBJ_IS_ARRAY(cx,obj)        (obj)->isArray()

As you can see OBJ_IS_DENSE_ARRAY() certainly does not make use of its cx parameter.  Can I just remove cx from this macro and from js_GetProtoIfDenseArray?

------
Two warnings from jslock.h from this source:

#define JS_IS_OBJ_LOCKED(cx,obj)        1
#define JS_IS_TITLE_LOCKED(cx,title)    1

The cx parameters aren't used. How to fix??  They are using in DEBUG but not release builds.
----------------
Lastly there are a bunch of warnings about Assignment operators
I need some ideas on what to do with these...

js\src\jshashtable.h(159) : warning C4512: 'js::detail::HashTable<T,HashPolicy,AllocPolicy>::Range' : assignment operator could not be generated
        with
        [
            T=JSObject *const ,
            HashPolicy=js::HashSet<JSObject *>::SetOps,
            AllocPolicy=js::ContextAllocPolicy
        ]
        c:\dev\common\spidermonkey\js\src\jshashtable.h(134) : see declaration of 'js::detail::HashTable<T,HashPolicy,AllocPolicy>::Range'
        with
        [
            T=JSObject *const ,
            HashPolicy=js::HashSet<JSObject *>::SetOps,
            AllocPolicy=js::ContextAllocPolicy
        ]
        c:\dev\common\spidermonkey\js\src\jshashtable.h(171) : see reference to class template instantiation 'js::detail::HashTable<T,HashPolicy,AllocPolicy>::Range' being compiled
        with
        [
            T=JSObject *const ,
            HashPolicy=js::HashSet<JSObject *>::SetOps,
            AllocPolicy=js::ContextAllocPolicy
        ]
        src\jshashtable.h(532) : see reference to class template instantiation 'js::detail::HashTable<T,HashPolicy,AllocPolicy>::Enum' being compiled
        with
        [
            T=JSObject *const ,
            HashPolicy=js::HashSet<JSObject *>::SetOps,
            AllocPolicy=js::ContextAllocPolicy
        ]
        js\src\jshashtable.h(868) : see reference to class template instantiation 'js::detail::HashTable<T,HashPolicy,AllocPolicy>' being compiled
        with
        [
            T=JSObject *const ,
            HashPolicy=js::HashSet<JSObject *>::SetOps,
            AllocPolicy=js::ContextAllocPolicy
        ]
        src/jscntxt.h(1363) : see reference to class template instantiation 'js::HashSet<T>' being compiled
        with
        [
            T=JSObject *
        ]


js/src/jscntxt.h(1813) : warning C4512: 'JSAutoIdArray' : assignment operator could not be generated 
js/src/jscntxt.h(1782) : see declaration of 'JSAutoIdArray'

js/src/jsscope.h(482) : warning C4512: 'JSScope' : assignment operator could not be generated
js/src/jsscope.h(211) : see declaration of 'JSScope'


js/src/jsscope.h(517) : warning C4512: 'JSEmptyScope' : assignment operator could not be generated
js/src/jsscope.h(484) : see declaration of 'JSEmptyScope'
Comment on attachment 429544 [details] [diff] [review]
Patch #2

Thanks for the patch -- drive-by nit-picking only:

>@@ -1260,5 +1260,10 @@
> 
> struct JSContext
> {
>+    #ifdef _MSC_VER
>+    #pragma warning(push)
>+    #pragma warning(disable:4355)
>+    #endif
>+    
>     explicit JSContext(JSRuntime *rt) : runtime(rt), busyArrays(this) {}
> 
>@@ -1263,5 +1268,9 @@
>     explicit JSContext(JSRuntime *rt) : runtime(rt), busyArrays(this) {}
> 
>+    #ifdef _MSC_VER
>+    #pragma warning(pop)
>+    #endif
>+

CPP # control lines start in column 1 in prevailing style, with any indentation after the # (only for locally nested #ifdefs, usually by 1 space increments).

>+++ b/js/src/json.cpp
>@@ -369,7 +369,7 @@
>     if (iterObj) {
>         // Always close the iterator, but make sure not to stomp on OK
>         JS_ASSERT(OBJECT_TO_JSVAL(iterObj) == *keySource);
>-        ok &= js_CloseIterator(cx, *keySource);
>+        ok &= (!!js_CloseIterator(cx, *keySource));

Right size of &= is overparenthesized.

Looks good otherwise -- maybe someone else (Igor has in the past) can give it a thumbs-up too.

/be
Attachment #429544 - Flags: review+
Attachment #429544 - Flags: review?(igor)
Thanks Mr B!
Good to hear from you.

Any ideas on stuff in comment #4?
Comment on attachment 429544 [details] [diff] [review]
Patch #2

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -5595,7 +5595,7 @@
>     save = cx->generatingError;
>     cx->generatingError = JS_TRUE;
>     ok = js_ReportUncaughtException(cx);
>-    cx->generatingError = save;
>+    cx->generatingError = !!save;

That should be fixed by changing the type for save from JSBool to bool not to waste any cycles to do the "!!" normalization. Note that this is not the patch problem. "!!" just exposes what the compiler otherwise doing in any case. But if we are touching the code it is better to fix the source of problems.

>     return ok;
> }
> 
>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp
>--- a/js/src/jscntxt.cpp
>+++ b/js/src/jscntxt.cpp
>@@ -1653,7 +1653,7 @@
>     PopulateReportBlame(cx, &report);
> 
>     if (!js_ExpandErrorArguments(cx, callback, userRef, errorNumber,
>-                                 &message, &report, charArgs, ap)) {
>+                                 &message, &report, !!charArgs, ap)) {
>         return JS_FALSE;
>     }

Here the warning means that the change from JSBool to bool in the bug 514585 was premature. To prevent that happen in future I suggest any change from JSBool to bool must be compiler with MSVC before landing to make sure that it is free of warnings.

>@@ -1260,5 +1260,10 @@
> 
> struct JSContext
> {
>+    #ifdef _MSC_VER
>+    #pragma warning(push)
>+    #pragma warning(disable:4355)
>+    #endif

Is it possible to fix the warning without the pragma by changing the order or something?

>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp
>--- a/js/src/jsdbgapi.cpp
>+++ b/js/src/jsdbgapi.cpp
>@@ -773,4 +773,7 @@
> {
>     if (sprop->attrs & JSPROP_SETTER) {
>         JSObject *funobj = sprop->setterObject();
>+        if (!funobj->isFunction())
>
>+            return false;
>
>+

This is not a warning fix. Why is this here?

>         JSFunction *fun = GET_FUNCTION_PRIVATE(cx, funobj);
>@@ -776,5 +779,4 @@
>         JSFunction *fun = GET_FUNCTION_PRIVATE(cx, funobj);
>-
>         return FUN_NATIVE(fun) == js_watch_set_wrapper;

Please avoid whitespace-only changes.

>     }
>     return sprop->setterOp() == js_watch_set;
>@@ -1463,7 +1465,7 @@
> {
>     pd->id = ID_TO_VALUE(sprop->id);
> 
>-    bool wasThrowing = cx->throwing;
>+    JSPackedBool wasThrowing = cx->throwing;

Use JSBool here.

>diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp
>+++ b/js/src/jsgc.cpp
>diff --git a/js/src/jsnum.cpp b/js/src/jsnum.cpp
>--- a/js/src/jsnum.cpp
>+++ b/js/src/jsnum.cpp
>@@ -457,8 +457,8 @@
>         return JS_TRUE;
> 
>     rt = cx->runtime;
>-    thousandsLength = strlen(rt->thousandsSeparator);
>-    decimalLength = strlen(rt->decimalSeparator);
>+    thousandsLength = char(strlen(rt->thousandsSeparator));
>+    decimalLength = char(strlen(rt->decimalSeparator));

The right fix here is to change thousandsLength and decimalLength and size to be size_t. 

>diff --git a/js/src/json.cpp b/js/src/json.cpp
>--- a/js/src/json.cpp
>+++ b/js/src/json.cpp
>@@ -369,7 +369,7 @@
>     if (iterObj) {
>         // Always close the iterator, but make sure not to stomp on OK
>         JS_ASSERT(OBJECT_TO_JSVAL(iterObj) == *keySource);
>-        ok &= js_CloseIterator(cx, *keySource);
>+        ok &= (!!js_CloseIterator(cx, *keySource));
>     }

The fix here is to change bool ok into JSBool ok.

> 
>     if (!ok)
>diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp
>--- a/js/src/jsopcode.cpp
>+++ b/js/src/jsopcode.cpp
>@@ -746,9 +746,9 @@
>     INIT_SPRINTER(cx, &jp->sprinter, &jp->pool, 0);
>     JS_InitArenaPool(&jp->pool, name, 256, 1, &cx->scriptStackQuota);
>     jp->indent = indent;
>-    jp->pretty = pretty;
>-    jp->grouped = grouped;
>-    jp->strict = strict;
>+    jp->pretty = !!pretty;
>+    jp->grouped = !!grouped;
>+    jp->strict = !!strict;
>     jp->script = NULL;
>     jp->dvgfence = NULL;
>     jp->pcstack = NULL;


Change here js_DecompileToString and js_NewPrinter to accept bool, not JSBool arguments.

>diff --git a/js/src/jsregexp.cpp b/js/src/jsregexp.cpp
> CalculateBitmapSize(CompilerState *state, RENode *target, const jschar *src,
>                     const jschar *end)
> {
>-    uintN max = 0;
>+    uint16 max = 0;

Fix the sites that assigns max, not its type.

>diff --git a/js/src/jsscan.cpp b/js/src/jsscan.cpp
>--- a/js/src/jsscan.cpp
>+++ b/js/src/jsscan.cpp
>@@ -650,7 +650,7 @@
>         ts->flags |= TSF_ERROR;
>     }
> 
>-    return warning;
>+    return !!warning;
> }

Make warning bool and fix few assignments to it to use false , not JS_FALSE.


>diff --git a/js/src/jsscopeinlines.h b/js/src/jsscopeinlines.h
>--- a/js/src/jsscopeinlines.h
>+++ b/js/src/jsscopeinlines.h
>@@ -130,7 +130,7 @@
>     if (!funobj)
>         return false;
>     *vp = OBJECT_TO_JSVAL(funobj);
>-    return js_SetPropertyHelper(cx, object, sprop->id, 0, vp);
>+    return !!js_SetPropertyHelper(cx, object, sprop->id, 0, vp);
> }

Too bad that we changed methodReadBarrier to be bool without compiling the code with MSVC...

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -2783,7 +2783,7 @@
>         if (JSDOUBLE_IS_INT(d, i))
>             goto store_int;
>       store_double:
>-        ok = js_NewDoubleInRootedValue(cx, d, &v);
>+        ok = !!js_NewDoubleInRootedValue(cx, d, &v);
>         if (!ok) {
>             js_ReportOutOfMemory(cx);
>             return false;

Change here ok to be JSBool.


>@@ -7799,7 +7807,7 @@
>     JSObject* obj2;
>     JSProperty* prop;
>     JSObject *obj = chainHead;
>-    bool ok = js_FindProperty(cx, ATOM_TO_JSID(atom), &obj, &obj2, &prop);
>+    bool ok = !!js_FindProperty(cx, ATOM_TO_JSID(atom), &obj, &obj2, &prop);

Make ok to be JSBool.

>@@ -13751,7 +13759,7 @@
> 
>     JSObject* obj2;
>     JSProperty* prop;
>-    bool ok = obj->lookupProperty(cx, id, &obj2, &prop);
>+    bool ok = !!obj->lookupProperty(cx, id, &obj2, &prop);
> 

Make ok to be JSBool.

I will r+ a patch that addresses these issue.
Attachment #429544 - Flags: review?(igor) → review-
(In reply to comment #4)
> Latest set of compile warnings fixed. New patch.
> Here are the remaining compile warnings from jstracer.cpp:
> 
> Should I just ignore the POD type warnings?  I think it would be nice if that
> could be fixed the right way since some compilers will may not initialize those
> structs like VC++ does.
> ------------------------------------
> jstracer.cpp(3867) : warning C4242: 'argument' : conversion from 'ptrdiff_t' to
> 'uint16', possible loss of data

We should cast here.

> jstracer.cpp(4084) : warning C4345: behavior change: an object of POD type
> constructed with an initializer of the form () will be default-initialized

Wow, this is a new one on me. "default-initialized" meaning givin a well-defined 0-ish initial value?

I defer to C++ mavens.

/be
>> jstracer.cpp(3867) : warning C4242: 'argument' : conversion from 'ptrdiff_t' to
>> 'uint16', possible loss of data

> We should cast here.

Their sizes are way different.  Do we really want to stuff that in there?

> Wow, this is a new one on me. "default-initialized" meaning givin a
> well-defined 0-ish initial value?

Apparently so.  This has more impact on VC++ 6.0 or older compilers that may not do this by default (and not tell you)
(In reply to comment #4)
> jstracer.cpp(3867) : warning C4242: 'argument' : conversion from 'ptrdiff_t' to

Please post the HG revision number to check the exact lines. jstracer.cpp frequently changes so there is no such thing as "the current tip"

> Source: 
> js_GetStackDefs(JSContext *cx, const JSCodeSpec *cs, JSOp op, JSScript
> *script,jsbytecode *pc)
> 
> (JSOp op) appears to only we used in this function for DEBUG builds.
> Can I just remove it completely?

SM relies on the compiler to eliminate the debug-only arguments from the inline functions. Don't VC have an option not to report such warnings?

> 
> -------
> 
> jsobj.h(191) : warning C4512: 'JSObjectMap' : assignment operator could not be
> generated  jsobj.h(184) : see declaration of 'JSObjectMap'

JSObjectMap is not supposed to be assigned. I guess this one can be fixed with explicit:

private:
  /* No copy or assignment semantics. */
  JSObjectMap(JSObjectMap &);
  void operator=(JSObjectMap &);

> Source:
> void
> JSStackFrame::assertValidStackDepth(uintN depth)
> {
>     JS_ASSERT(0 <= regs->sp - StackBase(this));
>     JS_ASSERT(depth <= uintptr_t(regs->sp - StackBase(this)));
> }
> 
> Can I just remove all references to this function during release builds?
> Why incur the cost to call to a function that does nothing???

Again we rely that the optimizing compiler would be able to eliminate a call to an empty inline function.


> As you can see OBJ_IS_DENSE_ARRAY() certainly does not make use of its cx
> parameter.  Can I just remove cx from this macro and from
> js_GetProtoIfDenseArray?

The macros are left for compatibility. If they generates warnings, then its time to remove them completely. But lets do that in another bug.
(In reply to comment #10)
> >> jstracer.cpp(3867) : warning C4242: 'argument' : conversion from 'ptrdiff_t' to
> >> 'uint16', possible loss of data
> 
> > We should cast here.
> 
> Their sizes are way different.  Do we really want to stuff that in there?

Yes, we do. We should assert that the value fits, if we don't already.

I don't know which line this is in your source, though -- can you cite it?

> > Wow, this is a new one on me. "default-initialized" meaning givin a
> > well-defined 0-ish initial value?
> 
> Apparently so.  This has more impact on VC++ 6.0 or older compilers that may
> not do this by default (and not tell you)

We don't support 6.0. But we can use C-style casts for POD. This is the first time I've heard of a reason to prefer C-style casts for POD. Luke, is this C++ standard practice?

/be
(In reply to comment #12)
> > > Wow, this is a new one on me. "default-initialized" meaning givin a
> > > well-defined 0-ish initial value?
> > 
> > Apparently so.  This has more impact on VC++ 6.0 or older compilers that may
> > not do this by default (and not tell you)
> 
> We don't support 6.0. But we can use C-style casts for POD. This is the first
> time I've heard of a reason to prefer C-style casts for POD. Luke, is this C++
> standard practice?

From a quick glance, it seems necessary that

4084:  GuardRecord* gr = new (traceAlloc()) GuardRecord();

default-initializes (aka, zero-initialize) the GuardRecord.  I think the warning was just saying that the older (broken) behavior was to leave gr uninitialized.  (I guess they were worried that people depended on this behavior and had already initialized the memory passed into placement new?)  Is there a way to silence the warning globally?
Attached patch Final patch? (obsolete) — — Splinter Review 
This patch fixes allwarnings in VC-2005.

Igor, Sorry for the patch bloat.

OBJ_IS_DENSE_ARRAY and OBJ_IS_ARRAY macro changes touched more code than I would have liked.  cx parameter was wastefully and not used.
 
Please no lets NOT open another bug for those fixes... :-)
Attachment #429544 - Attachment is obsolete: true
Attachment #429834 - Flags: review?(igor)
Comment on attachment 429834 [details] [diff] [review]
Final patch?

>diff --git a/js/src/jsapi.cpp b/js/src/jsapi.cpp
>--- a/js/src/jsapi.cpp
>+++ b/js/src/jsapi.cpp
>@@ -1139,9 +1139,9 @@
>     }
> 
>     /* Function.prototype and the global object delegate to Object.prototype. */
>-    OBJ_SET_PROTO(cx, fun_proto, obj_proto);
>-    if (!OBJ_GET_PROTO(cx, obj))
>-        OBJ_SET_PROTO(cx, obj, obj_proto);
>+    OBJ_SET_PROTO(fun_proto, obj_proto);
>+    if (!OBJ_GET_PROTO(obj))
>+        OBJ_SET_PROTO(obj, obj_proto);

Mike, please don't hack mega-patches that perpetuate old macros, at least not without more IRC consulting. We have C++ inline methods now. These should be obj->getProto(), etc. No more macros, leave the old ones to die, change uses as you go. Small patches are easier, always (I have to remind myself of this, I'm the worst offender).

Are you game to redo this to call the existing inline helpers?

/be
I've been thinking of converting the OBJ_IS_*ARRAY users but haven't gotten to it; in any case don't worry about not fixing up existing callers of them.
Brendan Yes I'm game.  
Please give me an example for the above.
Should I just remove the macros then?
(In reply to comment #17)
> Brendan Yes I'm game.  
> Please give me an example for the above.
> Should I just remove the macros then?

Yes, just remove the macros and replace them with obj->getClass|getProt etc.
(In reply to comment #18)
> (In reply to comment #17)
> > Brendan Yes I'm game.  
> > Please give me an example for the above.
> > Should I just remove the macros then?
> 
> Yes, just remove the macros and replace them with obj->getClass|getProt etc.

Also, I will review this promptly to avoid patch rusting. We should have done that long time ago.
Attached patch final-patch for real (obsolete) — — Splinter Review 
fixes old macro usage and uses new calls.
Patch got rather big...but its an easy read.
Attachment #429834 - Attachment is obsolete: true
Attachment #429928 - Flags: review?
Attachment #429834 - Flags: review?(igor)
Attachment #429928 - Flags: review? → review?(igor)
Comment on attachment 429928 [details] [diff] [review]
final-patch for real

>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp
>@@ -3515,7 +3515,7 @@
>             fprintf(stderr, "%s: not array\n", bytes);
>             cx->free(bytes);
>             continue;
>         }
>         fprintf(stderr, "%s: %s (len %lu", bytes,
>-                OBJ_IS_DENSE_ARRAY(cx, array) ? "dense" : "sparse",
>+                OBJ_IS_DENSE_ARRAY(array) ? "dense" : "sparse",
>                 array->fslots[JSSLOT_ARRAY_LENGTH]);
>@@ -3521,5 +3521,5 @@
>                 array->fslots[JSSLOT_ARRAY_LENGTH]);
>-        if (OBJ_IS_DENSE_ARRAY(cx, array)) {
>+        if (OBJ_IS_DENSE_ARRAY(array)) {


This still uses the macro form. Please make sure that the file compiles as a debug build.

>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp
>--- a/js/src/jsdbgapi.cpp
>+++ b/js/src/jsdbgapi.cpp
>@@ -773,6 +773,9 @@
> {
>     if (sprop->attrs & JSPROP_SETTER) {
>         JSObject *funobj = sprop->setterObject();
>+        if (!funobj->isFunction())
>+            return false;
>+

This change is still here in a supposedly warning-only fixing patch. If there is bug, please file one, but do not add unrelated changes here.

>diff --git a/js/src/jsobj.h b/js/src/jsobj.h
>--- a/js/src/jsobj.h
>+++ b/js/src/jsobj.h
>@@ -188,6 +188,11 @@
>     explicit JSObjectMap(const JSObjectOps *ops, uint32 shape) : ops(ops), shape(shape) {}
> 
>     enum { SHAPELESS = 0xffffffff };
>+
>+private:
>+    /* No copy or assignment semantics. */
>+    JSObjectMap(JSObjectMap &) : ops(NULL), shape(0) {};
>+    void operator=(JSObjectMap &){};

You do not need to write the implementation here. The idea is just to declare the constructor and operator=, not define them. This way any use of the constructor or an assignment will trigger an error even in the code that can access private members.

>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -97,6 +97,13 @@
> #include <elf.h>
> #endif
> 
>+/* Disable warning 4345: behavior change: an object of POD type constructed */
>+/* with an initializer of the form () will be default-initialized */
>+#ifdef _MSC_VER
>+#pragma warning(push)
>+#pragma warning(disable:4345) 
>+#endif

Please do not do this in this patch to keep warning for now to consider a proper fix later.

>+
> namespace nanojit {
> using namespace js;
> 
>@@ -107,7 +114,7 @@
> {
>     VMAllocator *vma = (VMAllocator*)this;
>     JS_ASSERT(!vma->outOfMemory());
>-    void *p = calloc(1, nbytes);
>+    void *p = js_calloc(nbytes);
>     if (!p) {
>         JS_ASSERT(nbytes < sizeof(vma->mReserve));
>         vma->mOutOfMemory = true;
>@@ -121,7 +128,7 @@
> nanojit::Allocator::freeChunk(void *p) {
>     VMAllocator *vma = (VMAllocator*)this;
>     if (p != &vma->mReserve[0])
>-        free(p);
>+        js_free(p);
> }
> 
> void
>@@ -2753,7 +2760,7 @@
> bool
> NativeToValue(JSContext* cx, jsval& v, TraceType type, double* slot)
> {
>-    bool ok;
>+    JSBool ok;
>     jsint i;
>     jsdouble d;
>     switch (type) {
>@@ -3549,9 +3556,9 @@
> 
>     /* Add the slot to the list of interned global slots. */
>     TraceType type;
>-    int index = tree->globalSlots->offsetOf(slot);
>+    int index = tree->globalSlots->offsetOf(uint16(slot));
>     if (index == -1) {
>         type = getCoercedType(*vp);
>         if (type == TT_INT32 && oracle.isGlobalSlotUndemotable(cx, slot))
>             type = TT_DOUBLE;
>         index = (int)tree->globalSlots->length();
>@@ -3553,9 +3560,9 @@
>     if (index == -1) {
>         type = getCoercedType(*vp);
>         if (type == TT_INT32 && oracle.isGlobalSlotUndemotable(cx, slot))
>             type = TT_DOUBLE;
>         index = (int)tree->globalSlots->length();
>-        tree->globalSlots->add(slot);
>+        tree->globalSlots->add(uint16(slot));
>         tree->typeMap.add(type);
>         SpecializeTreesToMissingGlobals(cx, globalObj, tree);
>         JS_ASSERT(tree->nGlobalTypes() == tree->globalSlots->length());
>@@ -3677,7 +3684,7 @@
>         return x;
>     if (isGlobal(p)) {
>         unsigned slot = nativeGlobalSlot(p);
>-        JS_ASSERT(tree->globalSlots->offsetOf(slot) != -1);
>+        JS_ASSERT(tree->globalSlots->offsetOf(uint16(slot)) != -1);
>         importGlobalSlot(slot);
>     } else {
>         unsigned slot = nativeStackSlot(p);
>@@ -3864,7 +3871,7 @@
>         if (i) {
>             m = isPromoteInt(i) ? TT_INT32 : TT_DOUBLE;
>         } else if (isGlobal(vp)) {
>-            int offset = tree->globalSlots->offsetOf(nativeGlobalSlot(vp));
>+            int offset = tree->globalSlots->offsetOf(uint16(nativeGlobalSlot(vp)));
>             JS_ASSERT(offset != -1);
>             m = importTypeMap[importStackSlots + offset];
>         } else {
>@@ -4353,6 +4360,10 @@
>     {
>     }
> 
>+    virtual ~SlotMap()
>+    {
>+    }
>+
>     JS_REQUIRES_STACK JS_ALWAYS_INLINE void
>     visitGlobalSlot(jsval *vp, unsigned n, unsigned slot)
>     {
>@@ -4415,7 +4426,7 @@
>             if (LIns* i = mRecorder.getFromTracker(vp)) {
>                 promoteInt = isPromoteInt(i);
>             } else if (mRecorder.isGlobal(vp)) {
>-                int offset = mRecorder.tree->globalSlots->offsetOf(mRecorder.nativeGlobalSlot(vp));
>+                int offset = mRecorder.tree->globalSlots->offsetOf(uint16(mRecorder.nativeGlobalSlot(vp)));
>                 JS_ASSERT(offset != -1);
>                 promoteInt = mRecorder.importTypeMap[mRecorder.importStackSlots + offset] ==
>                              TT_INT32;
>@@ -4520,6 +4531,10 @@
>     DefaultSlotMap(TraceRecorder& tr) : SlotMap(tr)
>     {
>     }
>+    
>+    virtual ~DefaultSlotMap()
>+    {
>+    }
> 
>     JS_REQUIRES_STACK JS_ALWAYS_INLINE bool
>     visitStackSlots(jsval *vp, size_t count, JSStackFrame* fp)
>@@ -7799,7 +7814,7 @@
>     JSObject* obj2;
>     JSProperty* prop;
>     JSObject *obj = chainHead;
>-    bool ok = js_FindProperty(cx, ATOM_TO_JSID(atom), &obj, &obj2, &prop);
>+    JSBool ok = js_FindProperty(cx, ATOM_TO_JSID(atom), &obj, &obj2, &prop);
> 
>     /* js_FindProperty can reenter the interpreter and kill |this|. */
>     if (!localtm.recorder)
>@@ -9132,5 +9147,5 @@
>     // hop along the proto chain when accessing a named (not indexed) property,
>     // typically to find Array.prototype methods.
>     JSObject* aobj = obj;
>-    if (OBJ_IS_DENSE_ARRAY(cx, obj)) {
>+    if (obj->isDenseArray()) {
>         guardDenseArray(obj, obj_ins, BRANCH_EXIT);
>@@ -9136,5 +9151,5 @@
>         guardDenseArray(obj, obj_ins, BRANCH_EXIT);
>-        aobj = OBJ_GET_PROTO(cx, obj);
>+        aobj = obj->getProto();
>         obj_ins = stobj_get_proto(obj_ins);
>     }
> 
>@@ -11270,7 +11285,7 @@
>     for (jsuword i = PCVCAP_TAG(entry->vcap) >> PCVCAP_PROTOBITS; i; i--)
>         obj2 = OBJ_GET_PARENT(cx, obj2);
>     for (jsuword j = PCVCAP_TAG(entry->vcap) & PCVCAP_PROTOMASK; j; j--)
>-        obj2 = OBJ_GET_PROTO(cx, obj2);
>+        obj2 = obj2->getProto();
>     scope = OBJ_SCOPE(obj2);
>     JS_ASSERT_IF(entry->adding(), obj2 == obj);
> 
>@@ -12103,7 +12118,7 @@
>         } else {
>             RETURN_STOP_A("can't trace setting typed array element to non-number value");
>         }
>-    } else if (JSVAL_TO_INT(idx) < 0 || !OBJ_IS_DENSE_ARRAY(cx, obj)) {
>+    } else if (JSVAL_TO_INT(idx) < 0 || !obj->isDenseArray()) {
>         CHECK_STATUS_A(initOrSetPropertyByIndex(obj_ins, idx_ins, &v,
>                                                 *cx->fp->regs->pc == JSOP_INITELEM));
>     } else {
>@@ -12461,7 +12476,7 @@
>     fi->pc = fp->regs->pc;
>     fi->imacpc = fp->imacpc;
>     fi->spdist = fp->regs->sp - fp->slots;
>-    fi->set_argc(argc, constructing);
>+    fi->set_argc(uint16(argc), constructing);
>     fi->callerHeight = stackSlots - (2 + argc);
>     fi->callerArgc = fp->argc;
> 
>@@ -12574,7 +12589,7 @@
>          * We trace dense arrays and arguments objects. The code we generate
>          * for apply uses imacros to handle a specific number of arguments.
>          */
>-        if (OBJ_IS_DENSE_ARRAY(cx, aobj)) {
>+        if (aobj->isDenseArray()) {
>             guardDenseArray(aobj, aobj_ins);
>             length = jsuint(aobj->fslots[JSSLOT_ARRAY_LENGTH]);
>             guard(true,
>@@ -13751,7 +13766,7 @@
> 
>     JSObject* obj2;
>     JSProperty* prop;
>-    bool ok = obj->lookupProperty(cx, id, &obj2, &prop);
>+    JSBool ok = obj->lookupProperty(cx, id, &obj2, &prop);
> 
>     /* lookupProperty can reenter the interpreter and kill |this|. */
>     if (!localtm.recorder) {
>@@ -14730,7 +14745,7 @@
>     JS_ASSERT(cx->fp->slots + slot < cx->fp->regs->sp - 1);
>     jsval &arrayval = cx->fp->slots[slot];
>     JS_ASSERT(JSVAL_IS_OBJECT(arrayval));
>-    JS_ASSERT(OBJ_IS_DENSE_ARRAY(cx, JSVAL_TO_OBJECT(arrayval)));
>+    JS_ASSERT(JSVAL_TO_OBJECT(arrayval)->isDenseArray());
>     LIns *array_ins = get(&arrayval);
>     jsval &elt = stackval(-1);
>     LIns *elt_ins = box_jsval(elt, get(&elt));
>@@ -14917,7 +14932,7 @@
>                                          NULL);
>         if (fun) {
>             funobj = FUN_OBJECT(fun);
>-            STOBJ_CLEAR_PROTO(funobj);
>+            funobj->clearProto();
>             STOBJ_CLEAR_PARENT(funobj);
> 
>             JS_LOCK_GC(rt);
>@@ -14993,8 +15008,8 @@
>     }
> 
>     LIns* v_ins;
>-    if (OBJ_IS_ARRAY(cx, obj)) {
>-        if (OBJ_IS_DENSE_ARRAY(cx, obj)) {
>+    if (obj->isArray()) {
>+        if (obj->isDenseArray()) {
>             if (!guardDenseArray(obj, obj_ins, BRANCH_EXIT)) {
>                 JS_NOT_REACHED("OBJ_IS_DENSE_ARRAY but not?!?");
>                 return ARECORD_STOP;
>@@ -15304,3 +15319,7 @@
> #include "jsrecursion.cpp"
> 
> } /* namespace js */
>+
>+#ifdef _MSC_VER
>+#pragma warning(pop)
>+#endif
>
>diff --git a/js/src/jstypedarray.cpp b/js/src/jstypedarray.cpp
>--- a/js/src/jstypedarray.cpp
>+++ b/js/src/jstypedarray.cpp
>@@ -255,7 +255,7 @@
>         return true;
>     }
> 
>-    JSObject *proto = STOBJ_GET_PROTO(obj);
>+    JSObject *proto = obj->getProto();
>     if (!proto) {
>         *objp = NULL;
>         *propp = NULL;
>@@ -401,7 +401,7 @@
>     }
> 
>     inline uint8_clamped& operator= (const jsdouble x) { 
>-        val = js_TypedArray_uint8_clamp_double(x);
>+        val = uint8(js_TypedArray_uint8_clamp_double(x));
>         return *this;
>     }
> 
>@@ -483,7 +483,7 @@
>             JSProperty *prop;
>             JSScopeProperty *sprop;
> 
>-            JSObject *proto = STOBJ_GET_PROTO(obj);
>+            JSObject *proto = obj->getProto();
>             if (!proto) {
>                 *vp = JSVAL_VOID;
>                 return true;
>@@ -1337,6 +1337,6 @@
> {
>     switch (atype) {
>       case TypedArray::TYPE_INT8:
>-        return Int8Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Int8Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       case TypedArray::TYPE_UINT8:
>@@ -1341,5 +1341,5 @@
> 
>       case TypedArray::TYPE_UINT8:
>-        return Uint8Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Uint8Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       case TypedArray::TYPE_INT16:
>@@ -1344,5 +1344,5 @@
> 
>       case TypedArray::TYPE_INT16:
>-        return Int16Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Int16Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       case TypedArray::TYPE_UINT16:
>@@ -1347,5 +1347,5 @@
> 
>       case TypedArray::TYPE_UINT16:
>-        return Uint16Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Uint16Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       case TypedArray::TYPE_INT32:
>@@ -1350,5 +1350,5 @@
> 
>       case TypedArray::TYPE_INT32:
>-        return Int32Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Int32Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       case TypedArray::TYPE_UINT32:
>@@ -1353,5 +1353,5 @@
> 
>       case TypedArray::TYPE_UINT32:
>-        return Uint32Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Uint32Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       case TypedArray::TYPE_FLOAT32:
>@@ -1356,5 +1356,5 @@
> 
>       case TypedArray::TYPE_FLOAT32:
>-        return Float32Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Float32Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       case TypedArray::TYPE_FLOAT64:
>@@ -1359,5 +1359,5 @@
> 
>       case TypedArray::TYPE_FLOAT64:
>-        return Float64Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Float64Array::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       case TypedArray::TYPE_UINT8_CLAMPED:
>@@ -1362,6 +1362,6 @@
> 
>       case TypedArray::TYPE_UINT8_CLAMPED:
>-        return Uint8ClampedArray::class_constructor(cx, cx->globalObject, argc, argv, rv);
>+        return !!Uint8ClampedArray::class_constructor(cx, cx->globalObject, argc, argv, rv);
> 
>       default:
>         JS_NOT_REACHED("shouldn't have gotten here");
>diff --git a/js/src/jsutil.h b/js/src/jsutil.h
>--- a/js/src/jsutil.h
>+++ b/js/src/jsutil.h
>@@ -179,6 +179,12 @@
> JS_DumpBacktrace(JSCallsite *trace);
> #endif
> 
>+#if defined JS_USE_CUSTOM_ALLOCATOR
>+
>+#include "jscustomallocator.h"
>+
>+#else
>+

Why this is here?


> static JS_INLINE void* js_malloc(size_t bytes) {
>     if (bytes < sizeof(void*)) /* for asyncFree */
>         bytes = sizeof(void*);
>@@ -200,6 +206,7 @@
> static JS_INLINE void js_free(void* p) {
>     free(p);
> }
>+#endif/* JS_USE_CUSTOM_ALLOCATOR */

And this?

>diff --git a/js/src/prmjtime.cpp b/js/src/prmjtime.cpp
>--- a/js/src/prmjtime.cpp
>+++ b/js/src/prmjtime.cpp
>@@ -43,6 +43,7 @@
> #ifdef SOLARIS
> #define _REENTRANT 1
> #endif
>+

Please remove this white-space only change.
Attachment #429928 - Flags: review?(igor) → review-
Attached patch final patch really this time... (obsolete) — — Splinter Review 
Removed extra stuff in the patch.
Put back 1 warning to tackle latter in another bug.
Attachment #429928 - Attachment is obsolete: true
Attachment #429953 - Flags: review?(igor)
(In reply to comment #22)
> Created an attachment (id=429953) [details]
> final patch really this time...
> 
> Removed extra stuff in the patch.
> Put back 1 warning to tackle latter in another bug.

That still contains JS_USE_CUSTOM_ALLOCATOR.
Attached patch two for the show... (obsolete) — — Splinter Review 
Removed extra file from patch.
Attachment #429953 - Attachment is obsolete: true
Attachment #429962 - Flags: review?(igor)
Attachment #429953 - Flags: review?(igor)
Attached patch updated to tip (obsolete) — — Splinter Review 
Updated patch for rot & tip changes.
Attachment #429962 - Attachment is obsolete: true
Attachment #429966 - Flags: review?(igor)
Attachment #429962 - Flags: review?(igor)
Attached patch Fix for malformed patch (obsolete) — — Splinter Review 
Fixed extra line feeds in patch.
Attachment #429966 - Attachment is obsolete: true
Attachment #429967 - Flags: review?(igor)
Attachment #429966 - Flags: review?(igor)
I tried the patch from the comment 26 and got:

~/m/tm/js/src> patch -i ~/s/msvc-warnings-07-patch -p3
patching file jsapi.cpp
patching file jsarray.cpp
Hunk #5 succeeded at 615 with fuzz 2.
Hunk #41 FAILED at 3521.
1 out of 42 hunks FAILED -- saving rejects to file jsarray.cpp.rej
patching file jsarray.h
Hunk #2 succeeded at 87 with fuzz 2.
patching file jsbuiltins.cpp
patching file jscntxt.cpp
patching file jscntxt.h
Hunk #4 FAILED at 1808.
1 out of 4 hunks FAILED -- saving rejects to file jscntxt.h.rej
patching file jsdbgapi.cpp
patching file jsfun.cpp
patching file jsgc.cpp
patching file jshashtable.h
patching file jsinterp.cpp
patching file jsinterp.h
Hunk #2 FAILED at 194.
1 out of 3 hunks FAILED -- saving rejects to file jsinterp.h.rej
patching file jsiter.cpp
patching file jsnum.cpp
patching file jsobj.cpp
patching file jsobj.h
patch: **** malformed patch at line 1063: @@ -362,7 +367,7 @@

My hg tip is:

~/m/tm/js/src> hg tip
changeset:   38541:0bb23f6b6333
tag:         tip
user:        Brendan Eich <brendan@mozilla.org>
date:        Tue Mar 02 17:48:00 2010 -0800
summary:     Typo'ed testname in jstests.list for 549617.

Mike, do you have other pathesin your queue? Also the malformed patch issue is strange one.
Attached patch malformed fix #2 (obsolete) — — Splinter Review 
My toirtoise hg diff tool is adding extra line feeds into some of the hunks!
Changed line counts for the hunk.  Should work now.

Also removed 1 file since patch applied to tip this morning not longer requires my fix.
My change set is: bc0a50164c21

Igor, can you run try server again?
If it fails can you send me the reject files.
Attachment #429967 - Attachment is obsolete: true
Attachment #430126 - Flags: review?(igor)
Attachment #429967 - Flags: review?(igor)
I am still getting:

~/m/tm/js/src> patch -i ~/s/msvc-warnings-08-patch -p3
patching file jsapi.cpp
patching file jsarray.cpp
Hunk #5 succeeded at 615 with fuzz 2.
Hunk #41 FAILED at 3521.
1 out of 42 hunks FAILED -- saving rejects to file jsarray.cpp.rej
patching file jsarray.h
Hunk #2 succeeded at 87 with fuzz 2.
patching file jsbuiltins.cpp
patching file jscntxt.cpp
patching file jscntxt.h
patching file jsdbgapi.cpp
patching file jsfun.cpp
patching file jsgc.cpp
patching file jsinterp.cpp
patching file jsinterp.h
Hunk #2 FAILED at 194.
1 out of 3 hunks FAILED -- saving rejects to file jsinterp.h.rej
patching file jsiter.cpp
patching file jsnum.cpp
patching file jsobj.cpp
patching file jsobj.h
patch: **** malformed patch at line 1034: @@ -362,7 +367,7 @@
Attached patch new patch different diff tool (obsolete) — — Splinter Review 
I reverted my changes to broken files. Then re-pull and re-patched by hand.
Then generated a patch with command line diff tool.
Not sure what else I can do!
Attachment #430126 - Attachment is obsolete: true
Attachment #430144 - Flags: review?(igor)
Attachment #430126 - Flags: review?(igor)
The patch causes build failures. The problem is that js/jsd/jsdebug.c (C file) includes jsopcode.h which now contains bool in js_NewPrinter. I will revert that to use JSBool and we will fix the warnings that this will induce in jsopcode.cpp in a separated patch.
Can I just fix jsdebug.c instead??? Would be better me thinks...
This has passed the try server. I will try to land it. The rest of the warnings we can track separately.
Attachment #430144 - Attachment is obsolete: true
Attachment #430515 - Flags: review+
Attachment #430144 - Flags: review?(igor)
Thanks igor!

I've already found a bunch more warning since that patch was created.
I'll file another bug later once this one has landed.
http://hg.mozilla.org/tracemonkey/rev/80644b76aa49
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/tracemonkey/rev/955f66e26af6 - I have landed an extra patch to eliminate PROTO access macros for the symmetry with changes from the previous landing.
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.

Attachment

General

Creator:
Created:
Updated:
Size: