Closed
Bug 584305
Opened 14 years ago
Closed 14 years ago
Error: Different types for "?:" (extern "C" xxxx) and xxxx
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)
Details
(Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css][suspect-regress-dromaeo_jslib])
Attachments
(1 file, 2 obsolete files)
8.19 KB,
patch
|
brendan
:
review+
luke
:
feedback+
|
Details | Diff | Splinter Review |
failed to compile with Sun Studio
Comment 2•14 years ago
|
||
Comment on attachment 462715 [details] [diff] [review] patch >diff --git a/js/src/jsgc.cpp b/js/src/jsgc.cpp >--- a/js/src/jsgc.cpp >+++ b/js/src/jsgc.cpp >- (op ? op : js_TraceObject)(trc, obj); >+ (op ? op : (JSTraceOp)js_TraceObject)(trc, obj); What exactly the error here? Also would using &js_TraceObject help?
"../../../mozilla-central/js/src/jsobj.cpp", line 1397: Error: Different types for "?:" (extern "C" int(*)(JSContext*,JSObject*,int,JSObject**,JSProperty**) and int(*)(JSContext*,JSObject*,int,JSObject**,JSProperty**)). JSTraceOp is defined as an extern "C" function pointer. js_TraceObject doesn't has extern "C". &js_TraceObject doesn't help.
Update to trunk
Attachment #462715 -
Attachment is obsolete: true
Attachment #476618 -
Flags: review?(igor)
Attachment #462715 -
Flags: review?(igor)
Comment 6•14 years ago
|
||
Comment on attachment 476618 [details] [diff] [review] patch >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp >--- a/js/src/jsobj.cpp >+++ b/js/src/jsobj.cpp >@@ -1411,17 +1411,17 @@ js_HasOwnPropertyHelper(JSContext *cx, J > return JS_TRUE; > } > > JSBool > js_HasOwnProperty(JSContext *cx, JSLookupPropOp lookup, JSObject *obj, jsid id, > JSObject **objp, JSProperty **propp) > { > JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED | JSRESOLVE_DETECTING); >- if (!(lookup ? lookup : js_LookupProperty)(cx, obj, id, objp, propp)) >+ if (!(lookup ? lookup : (JSLookupPropOp)js_LookupProperty)(cx, obj, id, objp, propp)) The casts look rather ugly, may hide type errors if some method signature changes and defeats an easy to read shortness of using (x ? y : z)(a, b, c) in place of x ? y(a, b, c) : z(a, b, c). So lets just replace the code with lookup ? lookup(cx, obj, id, objp, propp) : js_LookupProperty(cx, obj, id, objp, propp) with comments that refer to this bug that explains why the shorter form cannot be used.
Attachment #476618 -
Flags: review?(igor)
Comment 7•14 years ago
|
||
Must the typedefs in jspubtd.h be extern "C" still? I think we should lose that rather than crud up code. /be
I think adding extern "C" would be a better solution than comment #6. But it is also ugly, because some functions need to be extern "C", some functions should not.
Comment 9•14 years ago
|
||
Why is extern "C" needed at all? /be
Assignee | ||
Comment 10•14 years ago
|
||
If it is OK to remove extern "C" from jspubtd.h, it would be much easier. But they're JSAPI interfaces, can we remove extern "C" for them?
Comment 11•14 years ago
|
||
Can we do an "inside the C++ engine" vs. "outside in C API user land" distinction as Luke did for jsval / js::Value, etc.? /be
Comment 12•14 years ago
|
||
Yeah, we should be able to strip off the 'extern "C"' in the js::X versions of the JSX typedefs, or add them if they are missing. See the set of Jsvalify overloads in jsvalue.h.
Assignee | ||
Comment 13•14 years ago
|
||
Luke, is it what you suggested?
Attachment #476618 -
Attachment is obsolete: true
Attachment #477084 -
Flags: feedback?(lw)
Comment 14•14 years ago
|
||
Comment on attachment 477084 [details] [diff] [review] patch v2 Right on. >-js_HasOwnPropertyHelper(JSContext *cx, JSLookupPropOp lookup, uintN argc, >+js_HasOwnPropertyHelper(JSContext *cx, js::LookupPropOp lookup, uintN argc, >-js_HasOwnProperty(JSContext *cx, JSLookupPropOp lookup, JSObject *obj, jsid id, >+js_HasOwnProperty(JSContext *cx, js::LookupPropOp lookup, JSObject *obj, jsid id, Nit: don't need js:: prefix in jsobj.cpp; there is a using namespace js above.
Attachment #477084 -
Flags: feedback?(lw) → feedback+
Attachment #477084 -
Flags: review?(brendan)
Comment 15•14 years ago
|
||
Comment on attachment 477084 [details] [diff] [review] patch v2 Nice work, thanks a lot for doing this. Only these two ObjectOps remain extern "C", AFAICT: JSObjectOp thisObject; JSFinalizeOp clear; Ginn, want to take care of them too? I'll review but if it is a mechanical change as expected, rs=me (rs for "rubber stamp" ;-). /be
Attachment #477084 -
Flags: review?(brendan) → review+
Updated•14 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 16•14 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/20ebb001f3a9
Keywords: checkin-needed
Whiteboard: fixed-in-tracemonkey
Comment 17•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/20ebb001f3a9
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med]
Comment 18•14 years ago
|
||
A changeset from this bug was associated with a XP Ts, Cold MED Dirty Profile regression on Firefox. boo-urns :( Previous: avg 420.356 stddev 3.679 of 30 runs up to a9d1ad0bc386 New : avg 430.832 stddev 1.905 of 5 runs since a60414d076b5 Change : +10.475 (2.49% / z=2.847) Graph : http://mzl.la/bZFaB3 The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-ts_cold_generated_med] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med] → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css]
Comment 19•14 years ago
|
||
A changeset from this bug was associated with a Win7 Dromaeo (CSS) regression on Firefox. boo-urns :( Previous: avg 2014.419 stddev 40.480 of 30 runs up to a9d1ad0bc386 New : avg 1901.610 stddev 12.432 of 5 runs since a60414d076b5 Change : -112.809 (-5.6% / z=2.787) Graph : http://mzl.la/9gFu4n The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
Updated•14 years ago
|
Whiteboard: fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css] → fixed-in-tracemonkey[suspect-regress-ts_cold_generated_med][suspect-regress-dromaeo_css][suspect-regress-dromaeo_jslib]
Comment 20•14 years ago
|
||
A changeset from this bug was associated with a Win7 Dromaeo (jslib) regression on Firefox. boo-urns :( Previous: avg 127.610 stddev 4.222 of 30 runs up to a9d1ad0bc386 New : avg 116.384 stddev 0.751 of 5 runs since a60414d076b5 Change : -11.226 (-8.8% / z=2.659) Graph : http://mzl.la/bZu5UP The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
Comment 21•14 years ago
|
||
A changeset from this bug was associated with a XP Dromaeo (CSS) regression on Firefox. boo-urns :( Previous: avg 2045.275 stddev 49.676 of 30 runs up to a9d1ad0bc386 New : avg 1936.120 stddev 13.937 of 5 runs since a60414d076b5 Change : -109.155 (-5.34% / z=2.197) Graph : http://mzl.la/b0dlou The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-dromaeo_css] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
Comment 22•14 years ago
|
||
A changeset from this bug was associated with a XP Dromaeo (jslib) regression on Firefox. boo-urns :( Previous: avg 129.703 stddev 4.099 of 30 runs up to a9d1ad0bc386 New : avg 117.954 stddev 0.660 of 5 runs since a60414d076b5 Change : -11.749 (-9.06% / z=2.866) Graph : http://mzl.la/avZij4 The regression occurred from changesets in the following range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a9d1ad0bc386&tochange=a60414d076b5 The tag [suspect-regress-dromaeo_jslib] has been added to the status whiteboard; please remove it only once you have confirmed this bug is not the cause of the regression.
You need to log in
before you can comment on or make changes to this bug.
Description
•