Skip to content

multi: remove redundant condition in curl_multi_wait - #1439

Closed
sourcejedi wants to merge 1 commit into
curl:masterfrom
sourcejedi:patch-1
Closed

sourcejedi wants to merge 1 commit into
curl:masterfrom
sourcejedi:patch-1

Conversation

@sourcejedi

Copy link
Copy Markdown
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
Copy Markdown

@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.

@bagder bagder left a comment

Copy link
Copy Markdown
Member

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

bagder commented Apr 22, 2017

Copy link
Copy Markdown
Member

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.

3 participants