Closed Bug 1177892 Opened 9 years ago Closed 9 years ago

Remove *_TO_JSVAL

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(6 files)

First part removes BOOLEAN_TO_JSVAL and STRING_TO_JSVAL.
Attachment #8626721 - Flags: review?(evilpies)
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+
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)
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+
Keywords: leave-open
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)
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.
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.
The last one.
Attachment #8628113 - Flags: review?(evilpies)
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+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/799992ed5e8a
https://hg.mozilla.org/mozilla-central/rev/eb1cf46299e0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.