Move CloseSocket out of SetSocketNonBlocking and pass socket as const reference #10865
Conversation
theuni
left a comment
theuni
left a comment
There was a problem hiding this comment.
utACK. Can you please squash these together?
|
@theuni, I assume you mean squashing while merging, so nothing for me to do here? |
|
@corebob No, I mean combining the two commits into a single one, and force-pushing the result. Otherwise the first commit would technically be a regression. |
src/netbase.cpp
Outdated
There was a problem hiding this comment.
I guess the argument (to SetSocketNonBlocking) can be just SOCKET , or even const SOCKET now that it's no longer changed?
|
I think I got the squashing right, thanks for the assistance 😃 As far as I can see, SOCKET is defined as an unsigned int on both platforms, so passing it by value seems reasonable. It seems to me that both SetSocketNoDelay, InterrupibleRecv and IsSelectableSocket could benefit from const arguments as well. PS. I just realized I updated the patch with more code after approval. Maybe that was a bad idea. |
… reference in SetSocket* functions
|
@theuni Please comment if you have any issues with this. |
bytting
changed the title
|
This makes it more clear what's happening, looks good to me. utACK 05e023f |
…ocket as const reference 05e023f Move CloseSocket out of SetSocketNonBlocking and pass SOCKET by const reference in SetSocket* functions (Dag Robole) Pull request description: Rationale: Readability, SetSocketNonBlocking does what it says on the tin. Consistency, More consistent with the rest of the API in this unit. Reusability, SetSocketNonBlocking can also be used by clients that may not want to close the socket on failure. This also moves the responsibility of closing the socket back to the caller that opened it, which in general should know better how and when to close it. Tree-SHA512: 85027137f1b626e2b636549ee38cc757a587adcf464c84be6e65ca16e3b75d7ed1a1b21dd70dbe34c7c5d599af39e53b89932dfe3c74f91a22341ff3af5ea80a
|
ACK |
|
Postumous utACK |
… pass socket as const reference 05e023f Move CloseSocket out of SetSocketNonBlocking and pass SOCKET by const reference in SetSocket* functions (Dag Robole) Pull request description: Rationale: Readability, SetSocketNonBlocking does what it says on the tin. Consistency, More consistent with the rest of the API in this unit. Reusability, SetSocketNonBlocking can also be used by clients that may not want to close the socket on failure. This also moves the responsibility of closing the socket back to the caller that opened it, which in general should know better how and when to close it. Tree-SHA512: 85027137f1b626e2b636549ee38cc757a587adcf464c84be6e65ca16e3b75d7ed1a1b21dd70dbe34c7c5d599af39e53b89932dfe3c74f91a22341ff3af5ea80a
5 participants
Rationale:
Readability, SetSocketNonBlocking does what it says on the tin.
Consistency, More consistent with the rest of the API in this unit.
Reusability, SetSocketNonBlocking can also be used by clients that may not want to close the socket on failure.
This also moves the responsibility of closing the socket back to the caller that opened it, which in general should know better how and when to close it.