Closed Bug 1112769 Opened 7 years ago Closed 7 years ago

Move js/src/vm/NumericConversions.h to js/public/NumericConversions.h as public API


(Core :: JavaScript Engine, defect)

Not set





(Reporter: Waldo, Assigned: Waldo)




(1 file)

ToInt32, ToInteger, and so on are all widely used in ES3/5/6, and in WebIDL, and possibly in other web specs.  They deserve to be exposed as public API in namespace JS.
Attached patch PatchSplinter Review
It's mildly obnoxious that we have other functions named ToInt32 and so on, in a very few places, that take a Value and return a number via outparam, in namespace js.  The result is that inside namespace js, in the rare case where you're using one of these functions *not* by directly calling it, you'll need to explicitly request the namespace JS version if you want it.  This bit the few JS_FUNC_TO_DATA_PTR(void *, js::ToInt32) cases, that I had to convert to BitwiseCast<void*, int32_t(*)(double)>(JS::ToInt32), with the From type explicitly specified.  Not the end of the world.
Attachment #8538078 - Flags: review?(jorendorff)
Blocks: 1112774
Comment on attachment 8538078 [details] [diff] [review]

Review of attachment 8538078 [details] [diff] [review]:

To me, these two operations (converting a double to various integer types, which is concerned with numerical correctness; and coercing a Value to various integer types, which can execute arbitrary JS and possibly throw) are different enough that they should have different names. Would prefer JS::DoubleToInt32 and so on.

Alternatively, rename to js/public/Conversions.h with the understanding that each function will eventually get a fallible Value-oriented overload.
Attachment #8538078 - Flags: review?(jorendorff) → review+
I renamed to js/public/Conversions.h.  The functions' semantics are exactly those of the corresponding named algorithm in ES5/ES6, which seems to be no difference to me, except in invocation and error-checking and other window dressing.

I have a patch moving most of the rest of the other spec conversion methods into this header, that I'll throw into the other bug now.
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.