Closed
Bug 1177892
Opened 9 years ago
Closed 9 years ago
Remove *_TO_JSVAL
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
Details
Attachments
(6 files)
19.85 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
10.96 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
36.22 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
39.56 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
7.64 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
6.96 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
First part removes BOOLEAN_TO_JSVAL and STRING_TO_JSVAL.
Attachment #8626721 -
Flags: review?(evilpies)
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8626726 -
Flags: review?(evilpies)
Comment on attachment 8626721 [details] [diff] [review]
Part 1 - BOOLEAN_TO_JSVAL and STRING_TO_JSVAL
Review of attachment 8626721 [details] [diff] [review]:
-----------------------------------------------------------------
Nice
Attachment #8626721 -
Flags: review?(evilpies) → review+
Attachment #8626726 -
Flags: review?(evilpies) → review+
Assignee | ||
Comment 3•9 years ago
|
||
This one is a bit less mechanical. I used ObjectValue/setObject when it was clear from the code that the object must be non-null, else I used ObjectOrNullValue/setObjectOrNull.
Attachment #8626759 -
Flags: review?(evilpies)
Assignee | ||
Comment 4•9 years ago
|
||
I had to make Int32Value a constexpr for this to work.
Attachment #8626811 -
Flags: review?(evilpies)
Comment on attachment 8626759 [details] [diff] [review]
Part 3 - OBJECT_TO_JSVAL
Review of attachment 8626759 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/nsDOMClassInfo.cpp
@@ +1884,5 @@
> NS_ENSURE_TRUE(dot_prototype, NS_ERROR_OUT_OF_MEMORY);
> }
> }
>
> + v = JS::ObjectOrNullValue(dot_prototype);
Seems to me like dot_prototype is either non-null or initialized just above.
Attachment #8626759 -
Flags: review?(evilpies) → review+
Attachment #8626811 -
Flags: review?(evilpies) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 7•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
So DOUBLE_TO_JSVAL canonicalizes NaN and JS::DoubleValue doesn't. This is a pretty subtle difference, so I just renamed DOUBLE_TO_JSVAL to JS::CanonicalizedDoubleValue to make this more obvious.
I also changed some callers that pass in a definitely-not-NaN double to JS::DoubleValue instead of JS::CanonicalizedDoubleValue.
Attachment #8628098 -
Flags: review?(evilpies)
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8628098 [details] [diff] [review]
Part 5 - DOUBLE_TO_JSVAL
Review of attachment 8628098 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsapi.h
@@ +740,5 @@
> int32_t i;
> d = JS::CanonicalizeNaN(d);
> if (mozilla::NumberIsInt32(d, &i))
> return JS::Int32Value(i);
> + return JS::CanonicalizedDoubleValue(d);
Hm I just noticed we call CanonicalizeNaN above, so this could be JS::DoubleValue or we could remove the CanonicalizeNaN call. Changing this to JS::DoubleValue seems a little safer so I think I'll do that.
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8628098 [details] [diff] [review]
Part 5 - DOUBLE_TO_JSVAL
Review of attachment 8628098 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/public/Value.h
@@ +1959,5 @@
> UINT_TO_JSVAL(uint32_t i)
> {
> return i <= JSVAL_INT_MAX
> ? JS::Int32Value(int32_t(i))
> + : JS::CanonicalizedDoubleValue(double(i));
I'll change this to JS::DoubleValue, because an uint32_t is definitely not NaN.
Comment 14•9 years ago
|
||
Comment 15•9 years ago
|
||
Comment on attachment 8628098 [details] [diff] [review]
Part 5 - DOUBLE_TO_JSVAL
Review of attachment 8628098 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for introducing a safe constructor.
::: dom/bindings/Codegen.py
@@ +5573,5 @@
> return "JS::Int32Value(%s)" % (value.value)
> if tag == IDLType.Tags.uint32:
> return "UINT_TO_JSVAL(%sU)" % (value.value)
> if tag in [IDLType.Tags.int64, IDLType.Tags.uint64]:
> + return "JS::CanonicalizedDoubleValue(%s)" % numericValue(tag, value.value)
Oh .. int64 in JS :(
::: js/src/jsapi.h
@@ +740,5 @@
> int32_t i;
> d = JS::CanonicalizeNaN(d);
> if (mozilla::NumberIsInt32(d, &i))
> return JS::Int32Value(i);
> + return JS::CanonicalizedDoubleValue(d);
DoubleValue seems safer indeed. Shouldn't we maybe remove this function, there is some mismatch with JS::NumberValue here.
Attachment #8628098 -
Flags: review?(evilpies) → review+
Attachment #8628113 -
Flags: review?(evilpies) → review+
Comment 16•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
Keywords: leave-open
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/799992ed5e8a
https://hg.mozilla.org/mozilla-central/rev/eb1cf46299e0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in
before you can comment on or make changes to this bug.
Description
•