[SAB] Implement SharedArrayBuffer.prototype.slice

[SAB] Implement SharedArrayBuffer.prototype.slice

BUG=v8:5897
Review-Url: https://codereview.chromium.org/2741413006
Cr-Commit-Position: refs/heads/master@{#44075}
Committed: https://chromium.googlesource.com/v8/v8/+/fb52f5c55cb9bf8403931851b3a2d47512db45c2

[binji, 2017-03-13 18:41:12]

Description was changed from

==========
[SAB] Implement SharedArrayBuffer.prototype.slice

BUG=v8:5897
==========

to

==========
[SAB] Implement SharedArrayBuffer.prototype.slice

BUG=v8:5897
==========

[binji, 2017-03-13 18:41:12]

binji@chromium.org changed reviewers:
+ littledan@chromium.org

[binji, 2017-03-13 18:41:28]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 18:41:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 18:56:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 18:56:44]

Dry run: Try jobs failed on following builders:
  v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...)
  v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)

[binji, 2017-03-13 19:03:57]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 19:04:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-13 19:34:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 19:34:02]

Dry run: This issue passed the CQ dry run.

[binji, 2017-03-16 00:16:40]

ping

[Dan Ehrenberg, 2017-03-16 07:45:06]

Sorry for the delay here

https://codereview.chromium.org/2741413006/diff/20001/src/builtins/builtins-s...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2741413006/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer.cc:52:
BUILTIN(SharedArrayBufferPrototypeSlice) {
Would it be possible to reuse most of the implementation of
ArrayBufferPrototypeSlice, just with a different type check at the top?

[binji, 2017-03-16 16:17:00]

https://codereview.chromium.org/2741413006/diff/20001/src/builtins/builtins-s...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2741413006/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer.cc:52:
BUILTIN(SharedArrayBufferPrototypeSlice) {
On 2017/03/16 07:45:06, Dan Ehrenberg wrote:
> Would it be possible to reuse most of the implementation of
> ArrayBufferPrototypeSlice, just with a different type check at the top?

I thought so at first, but there are enough differences that it would make
things much harder to read IMO. For example, all the error messages  change, the
species constructor changes, the isShared checks change, and there is no check
for detached buffers either.

[Dan Ehrenberg, 2017-03-17 10:30:44]

lgtm

https://codereview.chromium.org/2741413006/diff/20001/src/builtins/builtins-s...
File src/builtins/builtins-sharedarraybuffer.cc (right):

https://codereview.chromium.org/2741413006/diff/20001/src/builtins/builtins-s...
src/builtins/builtins-sharedarraybuffer.cc:52:
BUILTIN(SharedArrayBufferPrototypeSlice) {
On 2017/03/16 16:17:00, binji wrote:
> On 2017/03/16 07:45:06, Dan Ehrenberg wrote:
> > Would it be possible to reuse most of the implementation of
> > ArrayBufferPrototypeSlice, just with a different type check at the top?
> 
> I thought so at first, but there are enough differences that it would make
> things much harder to read IMO. For example, all the error messages  change,
the
> species constructor changes, the isShared checks change, and there is no check
> for detached buffers either.

Well, I'd still would prefer for it to be de-duplicated. You could pass a flag
for which one it is to do the alternation. But we have a duplicated spec, so I
guess it's not unexpected to turn out like this.

[binji, 2017-03-17 18:58:55]

On 2017/03/17 10:30:44, Dan Ehrenberg wrote:
> lgtm
> 
>
https://codereview.chromium.org/2741413006/diff/20001/src/builtins/builtins-s...
> File src/builtins/builtins-sharedarraybuffer.cc (right):
> 
>
https://codereview.chromium.org/2741413006/diff/20001/src/builtins/builtins-s...
> src/builtins/builtins-sharedarraybuffer.cc:52:
> BUILTIN(SharedArrayBufferPrototypeSlice) {
> On 2017/03/16 16:17:00, binji wrote:
> > On 2017/03/16 07:45:06, Dan Ehrenberg wrote:
> > > Would it be possible to reuse most of the implementation of
> > > ArrayBufferPrototypeSlice, just with a different type check at the top?
> > 
> > I thought so at first, but there are enough differences that it would make
> > things much harder to read IMO. For example, all the error messages  change,
> the
> > species constructor changes, the isShared checks change, and there is no
check
> > for detached buffers either.
> 
> Well, I'd still would prefer for it to be de-duplicated. You could pass a flag
> for which one it is to do the alternation. But we have a duplicated spec, so I
> guess it's not unexpected to turn out like this.

OK, I'll try to dedupe.

[binji, 2017-03-22 19:12:08]

OK, deduped

[binji, 2017-03-22 19:12:12]

The CQ bit was checked by binji@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-22 19:12:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-22 19:40:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 19:40:22]

Dry run: This issue passed the CQ dry run.

[binji, 2017-03-23 17:57:40]

The CQ bit was checked by binji@chromium.org

[binji, 2017-03-23 17:57:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
littledan@chromium.org
Link to the patchset: https://codereview.chromium.org/2741413006/#ps40001
(title: "dedupe")

[commit-bot: I haz the power, 2017-03-23 17:57:48]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 18:13:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490291860848540,
"parent_rev": "d71ef941edca6ff99850a71ac146d9bbae5e61b7", "commit_rev":
"fb52f5c55cb9bf8403931851b3a2d47512db45c2"}

[commit-bot: I haz the power, 2017-03-23 18:13:31]

Description was changed from

==========
[SAB] Implement SharedArrayBuffer.prototype.slice

BUG=v8:5897
==========

to

==========
[SAB] Implement SharedArrayBuffer.prototype.slice

BUG=v8:5897

Review-Url: https://codereview.chromium.org/2741413006
Cr-Commit-Position: refs/heads/master@{#44075}
Committed:
https://chromium.googlesource.com/v8/v8/+/fb52f5c55cb9bf8403931851b3a2d47512d...
==========

[commit-bot: I haz the power, 2017-03-23 18:13:32]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/fb52f5c55cb9bf8403931851b3a2d47512d...