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

Fixed bug in Array.map and Array.indexedMap #171

Merged
merged 1 commit into from Feb 13, 2015

Conversation

Projects
None yet
5 participants
@brown-dragon
Contributor

brown-dragon commented Feb 13, 2015

Array.map and Array.indexedMap have a bug that can most easily be seen
with the following use case:

EXPECTED: Array.empty == Array.map (\e -> e) Array.empty
ACTUAL: Array.empty != Array.map (\e -> e) Array.empty

(the same for Array.indexedMap)

This is because Array.map/Array.indexedMap initializes itself
incorrectly using:

    table: new Array(a.table)  // creates a new array with an array element

instead of

    table: new Array(a.table.length) // a correctly-sized array

This bug has been fixed in this commit

Fix bug in Array.map and Array.indexedMap
Array.map and Array.indexedMap have a bug that can most easily be seen
with the following use case:

EXPECTED: Array.empty == Array.map (\e -> e) Array.empty
ACTUAL: Array.empty != Array.map (\e -> e) Array.empty

(the same for Array.indexedMap)

This is because Array.map/Array.indexedMap initializes itself
incorrectly using:

        table: new Array(a.table)  // creates a new array with an array element

instead of

        table: new Array(a.table.length) // a correctly-sized array

This bug has been fixed in this commit

@brown-dragon brown-dragon changed the title from Fix bug in Array.map and Array.indexedMap to Fixed bug in Array.map and Array.indexedMap Feb 13, 2015

@brown-dragon

This comment has been minimized.

Show comment
Hide comment
@brown-dragon

brown-dragon Feb 13, 2015

Contributor

I just realized this bug has been reported in https://github.com/elm-lang/core/pull/169

However this is a better fix, especially because the bug exists in all Array.map calls
(not just the empty case). It is most visible in the empty case but the initialization is
incorrect in all cases.

Contributor

brown-dragon commented Feb 13, 2015

I just realized this bug has been reported in https://github.com/elm-lang/core/pull/169

However this is a better fix, especially because the bug exists in all Array.map calls
(not just the empty case). It is most visible in the empty case but the initialization is
incorrect in all cases.

evancz pushed a commit that referenced this pull request Feb 13, 2015

Merge pull request #171 from brown-dragon/master
Fixed bug in Array.map and Array.indexedMap

@evancz evancz merged commit 445d76a into elm:master Feb 13, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Feb 13, 2015

Member

Awesome, thanks for the fix!

Member

evancz commented Feb 13, 2015

Awesome, thanks for the fix!

@TheSeamau5

This comment has been minimized.

Show comment
Hide comment
@TheSeamau5

TheSeamau5 Feb 16, 2015

Contributor

Yay! Awesome!

By the way, I just ran elm-check on the old code and it has found the bug.

Map transforms lists and arrays equally has failed with the following input: (<function>,Array.fromList [])

I know it's a bit of a shameless plug, but I think we could look into elm-check integration with the current testing of the standard library. I'm pretty surprised by how well elm-check is currently doing.

Contributor

TheSeamau5 commented Feb 16, 2015

Yay! Awesome!

By the way, I just ran elm-check on the old code and it has found the bug.

Map transforms lists and arrays equally has failed with the following input: (<function>,Array.fromList [])

I know it's a bit of a shameless plug, but I think we could look into elm-check integration with the current testing of the standard library. I'm pretty surprised by how well elm-check is currently doing.

@pchiusano

This comment has been minimized.

Show comment
Hide comment
@pchiusano

pchiusano Mar 9, 2015

Any chance of a 0.14.x patch release for this?

pchiusano commented Mar 9, 2015

Any chance of a 0.14.x patch release for this?

@kasbah

This comment has been minimized.

Show comment
Hide comment
@kasbah

kasbah Mar 10, 2015

Contributor

Prompted by you @pchiusano, I proposed it for next stable with #192.

Contributor

kasbah commented Mar 10, 2015

Prompted by you @pchiusano, I proposed it for next stable with #192.

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