-
Notifications
You must be signed in to change notification settings - Fork 401
Fix #1115: Enrich js.WrappedArray with Buffer and BufferLike. #1119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Review by @gzm0 |
|
Test PASSed. |
9bc7442 to
23f573e
Compare
|
Updated with the tests. |
23f573e to
0d2402b
Compare
|
Test FAILed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this, but I guess there is nothing we can do... Having js.Array.apply(x: Int) return js.UndefOr is too harsh...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already the problem with new js.Array[A](n) anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True
|
That's all |
|
Test PASSed. |
This is ok because the result() method of Builder clearly says that the builder's state is completely undefined after its invocation. For reference, scm.ArrayBuffer is also its own Builder.
This basically turns it into a full-blown Scala collection, whose implementation happens to use a js.Array. We also mark it @inline, because it gives the BufferLike interface to js.Array, which js.ArrayOps cannot provide due to the type parameters of BufferLike being too restrictive.
…rray. In a sense, this is as if .toJSArray were overridden in js.ArrayOps and js.WrappedArray, which makes total sense. But since .toJSArray is a pimp, we need to make this specialization directly in its implementation.
0d2402b to
2b5dc72
Compare
|
Updated. |
|
Would it be possible to patch the standard library Buffer/etc. to use On Fri, Sep 26, 2014 at 9:53 AM, Sébastien Doeraene <
|
|
LGTM |
|
Test PASSed. |
Fix #1115: Enrich js.WrappedArray with Buffer and BufferLike.
|
@lihaoyi Yes, actually, I think we could! It seems we just have to change the implementation of Then all y
8000
ou need to do in (cross-compiling) Scala code is use |
|
Test PASSed. |
With these changes, I was able to make the DeltaBlue benchmark run only 2.2x slowlier than the JS version, which means twice as fast as before. I managed this by replacing all Scala collections by
js.Arrays, and calling the methods ofjs.ArrayOpsandjs.WrappedArraydirectly on thejs.Arrays (which meant virtually no change but to the types).