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

Inbounds tuple iteration and reduction #26247

Closed
wants to merge 1 commit into from
Closed

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Feb 28, 2018

I noticed prod(length, (1,)) performed a bounds check, caused by the reduction not specifying @inbounds (which should be safe, because it is guarded by a call to done) and the tuple iteration functions not propagating it. Not sure about the correctness of this, so going for limited scope first.

vchuravy
vchuravy previously approved these changes Feb 28, 2018
@nalimilan
Copy link
Member

Unfortunately I'm not sure this is allowed in general. See discussion at #21402.

@maleadt
Copy link
Member Author

maleadt commented Feb 28, 2018

Hmm, I based it on existing code:

@inbounds (x, i) = next(itr, i)

@mbauman
Copy link
Member

mbauman commented Feb 28, 2018

Yes, we're inconsistent here… and at a standoff between two warring factions.

Part of the trouble is that next kinda got grandfathered in for iterating over Array since @inbounds used to propagate through all inlined functions for Array back in the dark ages. So when that got fixed, we hit perf regressions without them.

I'm on side @inbounds next.

@nalimilan
Copy link
Member

If we merge this I'll request merging #21402 too (which is very useful to to make checking the presence of missing values in a non-nullable array a no-op). :-p

@ararslan ararslan added the iteration Involves iteration or the iteration protocol label Feb 28, 2018
@vchuravy vchuravy self-requested a review April 24, 2018 14:33
@vchuravy
Copy link
Member

Hm somehow I can't dismiss my review ;)

@haampie
Copy link
Contributor

haampie commented Aug 26, 2018

Can be closed now that #28847 is merged

@mbauman mbauman closed this Aug 26, 2018
@nalimilan nalimilan deleted the tb/inbounds_tuple branch August 26, 2018 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iteration Involves iteration or the iteration protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants