Closed Bug 1112774 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: Waldo, Assigned: Waldo)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

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.
Depends on: 1112769
Attached patch Patch (obsolete) — Splinter Review
Attachment #8538082 - Flags: review?(jorendorff)
Attachment #8538082 - Flags: review?(jorendorff) → review+
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.
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.
Attachment #8544887 - Flags: review?(jorendorff)
Attachment #8538082 - Attachment is obsolete: true
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.
Attachment #8545273 - Flags: review?(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?
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.
Summary: Add js/public/Conversions.h, including NumericConversions.h and adding JS::ToObject as well → Move a bunch of conversion methods over into js/public/Conversions.h
(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.
Yeah, it made sense once you pointed it out.  And we continue to wish for some sort of encapsulating module system for C++...
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.
Attachment #8544887 - Flags: review?(jorendorff) → review+
Attachment #8545273 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/9df7385ff114
https://hg.mozilla.org/mozilla-central/rev/e87f763fc29a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
: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
Flags: needinfo?(jwalden+bmo)
No answer -> dev-doc-complete.
Flags: needinfo?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.