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

multi: remove redundant condition in curl_multi_wait #1439

Closed
wants to merge 1 commit into from

Conversation

sourcejedi
Copy link
Contributor

if(nfds || extra_nfds) { is followed by malloc(nfds * ...).

If extra_fs could be non-zero when nfds was zero, then we have malloc(0) which is allowed to return NULL. malloc returning NULL for success is a troublesome edge case. In this code, the next line would treat the NULL as an allocation failure.

It turns out, if nfds is zero then extra_nfds must also be zero. The final value of nfds includes extra_nfds. So the test for extra_nfds is redundant. It can only confuse the reader.

`if(nfds || extra_nfds) {` is followed by `malloc(nfds * ...)`.

If `extra_fs` could be non-zero when `nfds` was zero, then we have `malloc(0)` which is allowed to return `NULL`. But, malloc returning NULL can be confusing. In this code, the next line would treat the NULL as an allocation failure.

It turns out, if `nfds` is zero then `extra_nfds` must also be zero.  The final value of `nfds` includes `extra_nfds`.  So the test for `extra_nfds` is redundant.  It can only confuse the reader.
@mention-bot
Copy link

@sourcejedi, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @yangtse and @linusnielsen to be potential reviewers.

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Nice catch!

@bagder bagder closed this in be299a4 Apr 22, 2017
@bagder
Copy link
Member

bagder commented Apr 22, 2017

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators May 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants