Convert NativeArrayBuffer, NativeDataView, and TypedArrays to lambda #1709
Conversation
| double start = isArg(args, 0) ? ScriptRuntime.toNumber(args[0]) : 0; | ||
| double end = isArg(args, 1) ? ScriptRuntime.toNumber(args[1]) : self.getLength(); | ||
| int endI = | ||
| ScriptRuntime.toInt32( |
There was a problem hiding this comment.
I may be wrong here, but aren't most of the values here -- "self.getLength()" -- integers already? And is this method supposed to work if "start" and "end" are not integers? It looks like we are converting a bunch of integers into a floating-point value and then back to integers again.
Or are the "start" and "end" parameters here actually doubles because they could theoretically represent a value larger than 2^31? And if so can we handle that with our native byte arrays anyway?
In other words I'm asking if this whole thing shouldn't be done with integer arithmetic, and there may well be a reason!
There was a problem hiding this comment.
I'll have to look into it. I think I just copy pasted what was done before, but I'll see.
There was a problem hiding this comment.
It does allow for non-integers but they are supposed to get converted to integers before all the max/mins in the following lines.
There was a problem hiding this comment.
However, after seeing what tests failed, you were right. It looks like they can be more than 2^31, but in that case, the value would be greater than the length of the byte array, so when taking the min, it will just be the original byte array length, avoiding any issues.
| constructor.defineProperty( | ||
| cx, | ||
| "BYTES_PER_ELEMENT", | ||
| (Scriptable thisObj) -> BYTES_PER_ELEMENT, |
There was a problem hiding this comment.
Is there a reason why this is defined as a lambda function call -- it seems like rather than making the property value a function that returns a constant, can't we just set the value directly as an Integer object? This pattern happens in all the subclasses so my same comment would apply there too.
There was a problem hiding this comment.
Will change. Thanks
| scope, | ||
| "set", | ||
| 0, | ||
| (Context lcx, Scriptable lscope, Scriptable thisObj, Object[] args) -> { |
There was a problem hiding this comment.
Minor, but this seems like a bit too much code to be embedded inline like this, and it'd probably benefit from being a separate method.
This also updates the external-array harmony file taken from V8. There are still a few spots that error out with the new file, so I will get to those soon.
camnwalter
force-pushed
the
nativearray
branch
from
41c47e7
to
be08ffc
Compare
|
Rebased on master Will fix issues.. |
camnwalter
force-pushed
the
nativearray
branch
from
be08ffc
to
f32648b
Compare
camnwalter
force-pushed
the
nativearray
branch
from
f32648b
to
d8dc63d
Compare
|
Thanks -- this looks good! |
This PR converts the ArrayBuffer and adjacent classes to using LambdaConstructor.
I haven't added any new functionality, just a basic conversion from IdScriptable to using lambdas. Hopefully will get to adding more correct functionality in the future...
This should fix these inside #963 :
as they should all be accessor properties now.