1177892 - Remove *_TO_JSVAL

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Part 1 - BOOLEAN_TO_JSVAL and STRING_TO_JSVAL

First part removes BOOLEAN_TO_JSVAL and STRING_TO_JSVAL.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 2 - PRIVATE_TO_JSVAL

Comment 2

[Tom S [:evilpie]]

Comment on attachment 8626721 [details] [diff] [review]
Part 1 - BOOLEAN_TO_JSVAL and STRING_TO_JSVAL

Review of attachment 8626721 [details] [diff] [review]:
-----------------------------------------------------------------

Nice

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - OBJECT_TO_JSVAL

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.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - INT_TO_JSVAL

I had to make Int32Value a constexpr for this to work.

Comment 5

[Tom S [:evilpie]]

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.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/849151330d60
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad58c270ce87

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G bustage.
https://treeherder.mozilla.org/logviewer.html#?job_id=11223265&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe58c227072

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6e1b2b8ed9dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd794cb1591

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72f48fd13b90
https://hg.mozilla.org/integration/mozilla-inbound/rev/98495eee56f9

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 5 - DOUBLE_TO_JSVAL

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.

Comment 11

[Jan de Mooij [:jandem]]

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 12

[Jan de Mooij [:jandem]]

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 13

[Jan de Mooij [:jandem]]

Attachment: Part 6 - UINT_TO_JSVAL

The last one.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6e1b2b8ed9dd
https://hg.mozilla.org/mozilla-central/rev/1dd794cb1591
https://hg.mozilla.org/mozilla-central/rev/72f48fd13b90
https://hg.mozilla.org/mozilla-central/rev/98495eee56f9

Comment 15

[Tom S [:evilpie]]

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.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/799992ed5e8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb1cf46299e0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/799992ed5e8a
https://hg.mozilla.org/mozilla-central/rev/eb1cf46299e0