Bug 157407 – Clean up converting from JSValue to float / double in the bindings generator

• Component: Bindings
• Status: RESOLVED FIXED

[Chris Dumez]

Clean up converting from JSValue to float / double in the bindings generator.

[Chris Dumez]

Created attachment 278225 [details]
Patch

[Darin Adler]

Comment on attachment 278225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278225&action=review

> Source/WebCore/bindings/js/JSDOMBinding.h:746
> +template<typename T, ShouldAllowNonFinite allowNonFinite>
> +inline T toFloatingPoint(JSC::ExecState& state, JSC::JSValue value)

Another way to do this is to have this function be an overload of “build” rather than having floating point in the name. This has the same job as "build". There could be some benefit in doing that later, although we probably get no benefit from it now. I’m also willing to do something other than build.

> Source/WebCore/bindings/js/JSDOMBinding.h:751
> +        throwTypeError(&state, ASCIILiteral("The provided value is non-finite"));

I suggest making a non-inline function with this text in it to make the overall code size smaller.

[Darin Adler]

Comment on attachment 278225 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278225&action=review

>> Source/WebCore/bindings/js/JSDOMBinding.h:746
>> +inline T toFloatingPoint(JSC::ExecState& state, JSC::JSValue value)
> 
> Another way to do this is to have this function be an overload of “build” rather than having floating point in the name. This has the same job as "build". There could be some benefit in doing that later, although we probably get no benefit from it now. I’m also willing to do something other than build.

I meant: willing to rename "build" to something better. One name for all the "convert from JSValue in bindings" functions will have some benefits for some future meta-programming things even if the template arguments are different.

Given this is an inline function and constant folding, I think that ShouldAllowNonFinite could be a function argument rather than a template argument.

[Chris Dumez]

Created attachment 278259 [details]
Patch

[Chris Dumez]

@Darin: Is this what you had in mind?

FYI, I am not a huge fine of the build<>() naming in this context. I would have preferred something like toNative<>() or convertJSValue<>().

[Darin Adler]

Comment on attachment 278259 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=278259&action=review

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4411
> +        return ("build<double>(*state, $value, ShouldAllowNonFinite::No)", 1) if $type eq "double";
> +        return ("build<double>(*state, $value, ShouldAllowNonFinite::Yes)", 1) if $type eq "unrestricted double";
> +        return ("build<float>(*state, $value, ShouldAllowNonFinite::No)", 1) if $type eq "float";
> +        return ("build<float>(*state, $value, ShouldAllowNonFinite::Yes)", 1) if $type eq "unrestricted float";

Annoying that we have to write out all four cases, since the words "double" and "float" are the same as the arguments to build.

[Chris Dumez]

Created attachment 278273 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 278273 [details]
Patch

Clearing flags on attachment: 278273

Committed r200528: <http://trac.webkit.org/changeset/200528>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

I would be very happy to change the names build and JSDOMBuild.h to something better.

- Despite naming “toJS” I am not a big fan of “toXXX” names.

- I don’t think “native” is a great word because it is ambiguous. Could mean “native” ss in “native to JavaScript”.

- I don’t think we should put the type names in the function name, given how overloading works, so I wouldn't say “JSValue”.

- I considered the name “convert” instead of the name “build”. Maybe we should just switch to that?

[Chris Dumez]

(In reply to comment #10)
> I would be very happy to change the names build and JSDOMBuild.h to
> something better.
> 
> - Despite naming “toJS” I am not a big fan of “toXXX” names.
> 
> - I don’t think “native” is a great word because it is ambiguous. Could mean
> “native” ss in “native to JavaScript”.
> 
> - I don’t think we should put the type names in the function name, given how
> overloading works, so I wouldn't say “JSValue”.
> 
> - I considered the name “convert” instead of the name “build”. Maybe we
> should just switch to that?

Yes, I would prefer "convert" to "build" in this case.