Skip to content

Resolves #1391: Implement TypedArray updates to index handling. #2289

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 3, 2017
Merged

Resolves #1391: Implement TypedArray updates to index handling. #2289

merged 1 commit into from
Jan 3, 2017

Conversation

@atulkatti
Copy link
Contributor

@atulkatti atulkatti commented Dec 28, 2016 •
edited by dilijev
Loading

Resolves #1391: Implement TypedArray updates to index handling.

  1. Also, made changes to ArrayBuffer and DataView as per the spec updates.
  2. Added some more tests for DataView.

@msftclas
Copy link

msftclas commented Dec 28, 2016

Hi @atulkatti, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Atul Katti). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@atulkatti
Copy link
Contributor Author

atulkatti commented Dec 28, 2016

@Microsoft/chakra-developers

atulkatti
atulkatti commented Dec 28, 2016
View reviewed changes
test/typedarray/dataview.js
}

print("set little endian value offset " + offSet + " value " + value + " method " + setFuncs[i]);
print("set big endian value offset " + offSet + " value " + value + " method " + setFuncs[i]);
Copy link
Contributor Author

@atulkatti atulkatti Dec 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can ignore the changes to dataview.js, dataview.baseline and samethread.baseline. I fixed this typo in the comments of this test and that caused a lot of changes to the baseline. No other changes to these files.

@tcare
Copy link
Contributor

tcare commented Dec 28, 2016

Make sure to prefix the issue number with # so it triggers GitHub to link them.

tcare
tcare reviewed Dec 29, 2016
View reviewed changes
lib/Runtime/Library/TypedArray.cpp
newByteLength = mappedLength * elementSize;

if ((uint32)mappedLength > (byteLength - offset)/ elementSize)
if (offset + newByteLength > byteLength)
Copy link
Contributor

@tcare tcare Dec 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which case was this fixing?

Copy link
Contributor Author

@atulkatti atulkatti Dec 29, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed a slight change in the spec while reviewing the indexing related changes. Please refer to the spec at https://tc39.github.io/ecma262/#sec-typedarray-buffer-byteoffset-length section 22.2.4.5 step 12.

@atulkatti atulkatti changed the title Resolves 1391: Implement TypedArray updates to index handling. Resolves #1391: Implement TypedArray updates to index handling. Dec 29, 2016
@tcare tcare requested a review from boingoing December 30, 2016 18:31
@tcare
Copy link
Contributor

tcare commented Dec 30, 2016

@boingoing please take a look

@tcare
Copy link
Contributor

tcare commented Dec 30, 2016

@atulkatti please run buddy testing as per the email I sent

@chakrabot chakrabot merged commit 17b8f3a into chakra-core:release/1.4 Jan 3, 2017
chakrabot pushed a commit that referenced this pull request Jan 3, 2017
[MERGE #2289 @atulkatti] Resolves #1391: Implement TypedArray updates…
d0f4f88 
… to index handling.

Merge pull request #2289 from atulkatti:TypedArray.Issue1391

Resolves #1391: Implement TypedArray updates to index handling.
1. Also, made changes to ArrayBuffer and DataView as per the spec updates.
2. Added some more tests for DataView.
chakrabot pushed a commit that referenced this pull request Jan 3, 2017
[1.4>master] [MERGE #2289 @atulkatti] Resolves #1391: Implement Typed…
26e3ccd 
…Array updates to index handling.

Merge pull request #2289 from atulkatti:TypedArray.Issue1391

Resolves #1391: Implement TypedArray updates to index handling.
1. Also, made changes to ArrayBuffer and DataView as per the spec updates.
2. Added some more tests for DataView.
@kunalspathak kunalspathak mentioned this pull request Jan 4, 2017
Closed
@tcare tcare mentioned this pull request Jan 11, 2017
Merged
@ljharb
Copy link
Collaborator

ljharb commented Jan 11, 2017

(Reverted by #2350)

@dilijev dilijev mentioned this pull request Jan 11, 2017
Closed
@dilijev
Copy link
Contributor

dilijev commented Jan 11, 2017

@ljharb @tcare FYI #1391 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dilijev dilijev Awaiting requested review from dilijev

@ianwjhalliday ianwjhalliday Awaiting requested review from ianwjhalliday

@boingoing boingoing Awaiting requested review from boingoing

1 more reviewer

@tcare tcare tcare left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@atulkatti @msftclas @tcare @ljharb @dilijev @chakrabot