1112774 - Move a bunch of conversion methods over into js/public/Conversions.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

The non-exposure of the ES5 ToObject algorithm is a criminal lapse in the JSAPI.  Rectify this with a new js/Conversions.h header that also includes js/NumericConversions.h (but leaves that separate so that including it can be a lighter-weight thing).

I expect we'll add other conversion operations (JS::ToString, for example) to Conversions.h over time.  But this is all I need right now, so I'm stopping here.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Jason Orendorff [:jorendorff]]

Note that the review in bug 1112769 comment 2 requires either renaming the existing header to Conversions.h or renaming the functions --- here it looks like you chose to rename the functions, which is a bit of a surprise given our conversation yesterday.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Add a raft of other conversion methods to js/public/Conversions.h past just JS::ToObject

I went back and moved a whole bunch more methods into Conversions.h to make things consistent.  No renames of anything, just API moves.

I'm not sure how you read the prior patch here to rename any functions -- this was pure moving as far as I can tell.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Remove JS_DoubleTo{Ui,I}nt32 in favor of JS::To{Ui,I}nt32

More small consolidation.  I see no reason to have these given the other methods exist now, with the names from the ES6 standard that people would expect.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> I'm not sure how you read the prior patch here to rename any functions --
> this was pure moving as far as I can tell.

What I meant is, you did not choose to renaming the existing header to Conversions.h. Therefore I guess you chose renaming the functions? I don't know, Waldo, you tell me. What's going on here?

Comment 6

[Jeff Walden [:Waldo]]

Oh, you mean having *both* NumericConversions.h and Conversions.h, I guess.  The idea was that narrower uses that didn't need the full contents of the header, could be a little simpler by referring only to the finer-grained header.  Perhaps this was *too* fine a splitting.

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #6)
> Oh, you mean having *both* NumericConversions.h and Conversions.h, I guess. 
> The idea was that narrower uses that didn't need the full contents of the
> header, could be a little simpler by referring only to the finer-grained
> header.  Perhaps this was *too* fine a splitting.

Oh --- to clarify, the reason I asked for both signatures to be declared in the same header was so that it would be impossible to include one signature without the other.

Otherwise, it's possible for a cast (or call to a function template) that works in a non-unified build, because only one signature is visible, to break in a unified build, where all signatures have been magically included.

Comment 8

[Jeff Walden [:Waldo]]

Yeah, it made sense once you pointed it out.  And we continue to wish for some sort of encapsulating module system for C++...

Comment 9

[Jason Orendorff [:jorendorff]]

Comment on attachment 8544887 [details] [diff] [review]
Add a raft of other conversion methods to js/public/Conversions.h past just JS::ToObject

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

Thanks.

::: js/public/Conversions.h
@@ +23,5 @@
> +struct JSContext;
> +
> +namespace js {
> +
> +/* DO NOT CALL THIS. Use JS::ToBoolean. */

It'd be nice to move this stuff into JS::detail instead of using comments, someday.

::: js/src/jsobj.cpp
@@ +3625,4 @@
>   * Callers must handle the already-object case.
>   */
>  JSObject *
> +js::ToObjectSlow(JSContext *cx, JS::HandleValue val, bool reportScanStack)

Huh. Why was this necessary? Seems like HandleValue is used unqualified all over in jsobj.cpp.

Comment 10

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9df7385ff114
https://hg.mozilla.org/integration/mozilla-inbound/rev/e87f763fc29a

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9df7385ff114
https://hg.mozilla.org/mozilla-central/rev/e87f763fc29a

Comment 12

[Florian Scholz (Open Web Docs)]

:Waldo, the following docs got updated to say that JS::To{Ui,I}nt32 are obsolete starting with SpiderMonkey 38. Can you review, and mark this as dev-doc-complete if that is all we need docs-wise here? Thanks!

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_DoubleToInt32
https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference#Numbers

Comment 13

[Florian Scholz (Open Web Docs)]

No answer -> dev-doc-complete.