Skip to content

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Feb 10, 2019

@pablogsal
Copy link
Member Author

CC @tirkarthi @remilapeyre

@remilapeyre
Copy link

remilapeyre commented Feb 10, 2019

This fix the issue, do you think we still need to consume the whole iterable thought?

Maybe we could break early?

@pablogsal
Copy link
Member Author

pablogsal commented Feb 10, 2019

This fix the issue, do you think we still need to consume the whole iterable thought? Maybe we could break early?

If something like ('nan') appear afterwards, the result need to be ('nan') so we cannot break earlier.

@remilapeyre
Copy link

You're right, thanks.

prod([10, 20], [30, 40]) # start is a keyword-only argument

self.assertEqual(prod([0, 1, 2, 3]), 0)
self.assertEqual(prod([1, 0, 2, 3]), 0)
Copy link
Member

Choose a reason for hiding this comment

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

[1, 2, 3, 0] was already working fine without this fix so is it worth adding this as a test case? I have less knowledge with C so please ignore if this is not needed.

self.assertEqual(prod([1, 2, 3, 0]), 0)

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added this test and removed the range base one. Even if that case worked fine because it takes a different code path, I like to have the 0 in the start/middle/end.

Thanks for the catch!

Copy link
Member

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

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

This PR fixes my case. Thanks for the explanation on the zero by division code path.

@pablogsal
Copy link
Member Author

@tirkarthi Thanks to you for finding the bug! :)

@pablogsal pablogsal merged commit 4207907 into python:master Feb 10, 2019
@pablogsal pablogsal deleted the bpo35959 branch February 10, 2019 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants