neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/19415 )
Change subject: socket: add osmo_sockaddr_cmp() ...................................................................... Patch Set 12: Code-Review-1 (10 comments) https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c File src/socket.c: https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1562 PS11, Line 1562: /*! Compare two osmo_sockaddr. Return 0 if they are same "the same". End with a ".". Actually the \return is already below. https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1563 PS11, Line 1563: * \brief osmo_sockaddr_cmp we use AUTO_BRIEF, so drop this \brief line https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1566 PS11, Line 1566: * \return 0 if a and b are equal. the naming of "cmp" implies being able to use the return code for sorting. So please return 1 if a > b and -1 if a < b. If you don't want to do that, s/_cmp/_same https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1568 PS11, Line 1568: int osmo_sockaddr_cmp(struct osmo_sockaddr *a, struct osmo_sockaddr *b) > As usual, I see no point in checking and returning stuff for NULL pointers. > […] I strongly favor generic cmp functions being useful with NULL args as well, it's not much effort and gives a warm and comfortable peace of mind https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1570 PS11, Line 1570: if (!a && !b) this could be just if (a == b) return 0; https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1573 PS11, Line 1573: return 1; -1 https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1577 PS11, Line 1577: return 1; return OSMO_CMP(a->.., b->..) https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1583 PS11, Line 1583: a->u.sin.sin_port == b->u.sin.sin_port); cleaner, type wise, would be to return <condition> ? 0 : 1 not sure how to do proper cmp rc of -1/1 here... at least the port could use OSMO_CMP() https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1585 PS11, Line 1585: /* AF_INET6 usally not contain any padding */ "does not" https://gerrit.osmocom.org/c/libosmocore/+/19415/11/src/socket.c@1588 PS11, Line 1588: /* fallback to memcmp for remaing AF */ "remaining" -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/19415 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I2d12ebae2710ffd17cf071e6ada0804e73f87dd6 Gerrit-Change-Number: 19415 Gerrit-PatchSet: 12 Gerrit-Owner: lynxis lazus <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: lynxis lazus <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Comment-Date: Thu, 27 Aug 2020 11:14:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
