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.

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

StaticArray/Array improvements (map_with_index!) 2 #4456

Merged
merged 3 commits into from May 26, 2017
Merged

StaticArray/Array improvements (map_with_index!) 2 #4456

merged 3 commits into from May 26, 2017

Conversation

Nephos
Copy link
Contributor

@Nephos Nephos commented May 24, 2017

cf: #3356
(misusage of git on my side, I had to recreate a new PR, my bad)

Aims: have map_with_index! in the StaticArray

@oprypin
Copy link
Member

oprypin commented May 24, 2017

I don't understand, why is StaticArray so special that you're making it the only class that has map_with_index!?

@mverzilli
Copy link

It should be map_with_index (without !), right?

@mverzilli
Copy link

Nevermind, I got confused. It's with ! because it ends up calling to_unsafe.map!, which mutates. @oprypin are you objecting against this PR not including a map_with_index! for the common Array?

@Nephos
Copy link
Contributor Author

Nephos commented May 24, 2017

It would be cool to have map_with_index! in the Array too

@mverzilli
Copy link

Cool! Would you add map_with_index! to Array then?

Add Array(U).map_with_index!
5100598 
After adding map_with_index! to StaticArray(T, I), it is good to have the same
behaviour for Array(T).

* Add Array(T).map_with_index!(&block)
* Add a spec to test it
@Nephos
Copy link
Contributor Author

Nephos commented May 25, 2017

Anything else that should be added / changed ?

@mverzilli
Copy link

No, looks good to me! Thank you!

@bcardiff
Copy link
Member

Since map! is actually implemented in Pointer, why not implement there the logic and let StaticArray forward the call as in map!

Also, in Array#map_with_index! I think there should be no U, just T, since it is an inplace replacement. Forcing a T in the block will give a different and clearer error message.

Add Pointer.map_with_index!
de54675 
map! is defined first in Pointer, so map_with_index! should be.

* move map_with_index! logic from Array/StaticArray to Pointer
* fix map_with_index! parameters (block arguments / return)
* add spec for Pointer.map_with_index!
@Nephos
Copy link
Contributor Author

Nephos commented May 26, 2017

You're right ! I fix that.

@Nephos Nephos changed the title StaticArray improvements (map_with_index!) 2 StaticArray/Array improvements (map_with_index!) 2 May 26, 2017
@mverzilli mverzilli added this to the Next milestone May 26, 2017
@mverzilli mverzilli merged commit a6a4f22 into crystal-lang:master May 26, 2017
@bew
Copy link
Contributor

bew commented May 26, 2017

In StaticArray#map_with_index!, Pointer#map_with_index! & Array#map_with_index! the yield keywork should be replaced by block.call IMO.
Or remove the block parameter, and use only yield ?

We shouldn't mix named block & yield.

@RX14
Copy link
Contributor

RX14 commented May 26, 2017

The block parameter is only used as documentation for the block types. Using block.call instead of yield will be uglier and perform much much worse.

@bew
Copy link
Contributor

bew commented May 26, 2017

I see, for Pointer#map_with_index! the block argument has no documentation, but is still there though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
kind:feature status:accepted topic:stdlib
Projects
None yet
Milestone
0.23.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Nephos @oprypin @mverzilli @bcardiff @bew @RX14