Skip to content
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

Editorial: simplify ArraySetLength truncation #1702

Merged
merged 1 commit into from Nov 19, 2019

Conversation

@devsnek
Copy link
Member

devsnek commented Sep 19, 2019

This change iterates over the existing keys of an array when truncating it instead of every possible key between newLen and oldLen, clarifying the observable behaviour of the loop.

https://deploy-preview-1702--ecma262-snapshots.netlify.com/#sec-arraysetlength

The behaviour of this change has been verified in engine262.

@devsnek devsnek force-pushed the devsnek:refactor/array-truncate branch from 5debbae to 9fa27e7 Sep 19, 2019
spec.html Outdated Show resolved Hide resolved
@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented Sep 19, 2019

Happy to see the spec becoming closer to what implementations already do internally :)

spec.html Outdated
1. Repeat, while _newLen_ < _oldLen_,
1. Set _oldLen_ to _oldLen_ - 1.
1. Let _deleteSucceeded_ be ! _A_.[[Delete]](! ToString(_oldLen_)).
1. For each own property key _P_ of _A_ that is an array index and is greater than or equal to _newLen_, in descending numeric index order, do

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 19, 2019

Member

prose seems like a strange way to express this; why not something like (pseudocode):

1. Let _ownArrayIndices_ = GetOwnArrayIndices(_A_).
1. For each _P_ in _ownArrayIndices_ in List order, do

?

This comment has been minimized.

Copy link
@devsnek

devsnek Sep 19, 2019

Author Member

I based it on For each own property key _P_ of _A_ that is an array index, in ascending numeric index order from OrdinaryOwnPropertyKeys

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Sep 19, 2019

This seems like it would be an observable change in theory, even if in practice all engines already implement it like the PR - which would make this normative / web reality?

@devsnek

This comment has been minimized.

Copy link
Member Author

devsnek commented Sep 19, 2019

@ljharb as far as i'm aware this diff isn't observable.

@devsnek devsnek force-pushed the devsnek:refactor/array-truncate branch from 9fa27e7 to 0a0fa4d Sep 19, 2019
@bakkot
bakkot approved these changes Nov 13, 2019
Copy link
Contributor

bakkot left a comment

Working with the underlying collection of properties instead of working via the MOP is always a little scary, but I think it's appropriate here, since this is in a method which is exclusively for use by a MOP operation on array exotic objects.

This looks like a solid improvement in clarity with no observable change in behavior. LGTM.

@ljharb ljharb requested a review from syg Nov 13, 2019
@syg
syg approved these changes Nov 19, 2019
@ljharb ljharb force-pushed the devsnek:refactor/array-truncate branch from 0a0fa4d to cc2312d Nov 19, 2019
@ljharb
ljharb approved these changes Nov 19, 2019
Copy link
Member

ljharb left a comment

still not stoked on the prose here, but it can be improved in multiple places at once in a future PR.

@ljharb ljharb merged commit cc2312d into tc39:master Nov 19, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/ecma262-snapshots/deploy-preview Deploy preview ready!
Details
@devsnek devsnek deleted the devsnek:refactor/array-truncate branch Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.