connect: remove superfluous conditional #7511
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commit dbd16c3 cleaned up the logic for traversing the addrinfos, but the move left a conditional on ai which no longer is needed as the while loop reevaluation will cover it. Closes #xxxx
carenas
reviewed
Jul 29, 2021
| result = singleipconnect(data, conn, ai, tempindex); | ||
| if(result == CURLE_COULDNT_CONNECT) { | ||
| ai = ainext(conn, tempindex, TRUE); | ||
| continue; |
There was a problem hiding this comment.
continue and break could be removed by adding "&& result == CURLE_COULDNT_CONNECT" to the while loop condition
There was a problem hiding this comment.
It could, but it would have to be a do .. while loop instead as you'd otherwise inspect result before it's been set by the logic in question. Not sure that improves readability.
There was a problem hiding this comment.
result is already initialized to the right value in line 579, so there is no need to change the loop AFAIK and IMHO makes the code more readable by meaning: "while there are still addresses to check and we hadn't be able to connect do ..."
|
result is already initialized to the right value in line 579, so there is no need to change the loop
It is, but there is no guarantee that there wont be code inserted between there and here which scribbles over result (and which misses to update the loop conditional).
IMHO makes the code more readable by meaning: "while there are still addresses to check and we hadn't be able to connect do ..."
A do .. while loop would satisfy that even better then, since the connection hasn't been attempted at that point.
|
bagder
approved these changes
Jul 29, 2021
|
@bagder approved this pull request.
👍 on the submitted suggestion. Not convinced about the suggested further cleanup, but shown the suggestion as a PR I might change my mind.
Pushed like that.
|
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Commit dbd16c3 cleaned up the logic for traversing the addrinfos,
but the move left a conditional on ai which no longer is needed as
the while loop reevaluation will cover it.
Closes #xxxx