Patch Set 13: Code-Review-1 (10 comments)
https://gerrit.osmocom.org/#/c/4006/13/src/libosmo-mgcp/mgcp_sdp.c File src/libosmo-mgcp/mgcp_sdp.c: Line 41: /*! set codec configuration depending on payload type and codec name. Nice, ends in a full stop, now all you need is to start with a capital letter :) Line 42: * \endp[in] ctx talloc context \param Line 147: int payload, char *audio_name) Let's constify all pointer args that aren't modified. Line 195: * \endp[in] endp trunk endpoint that should be '\param', not '\endp' Line 197: * \endp[in] caller provided memory to store the parsing results technically, that's an [out], then. Plus, there is no 'caller' arg in the function signature. Line 198: * \returns 0 on success, -1 on failure */ it would be good to always include a '.' at the end of each \param, each \returns and each sentence in general. In doxygen, line breaks will not be visible, adding '.' clarifies things. Line 200: struct mgcp_parse_data *p) Let's constify all pointer args that aren't modified. Line 328: /*! generate SDP response string. I wonder if this should use msgb_printf() that we added recently instead of returning to a fixed string buffer. Line 329: * \endp[in] endp trunk endpoint \param Line 335: int mgcp_write_response_sdp(struct mgcp_endpoint *endp, > not sure if endp and/or conn could be 'const' (read-only), but this is not since it's public API the good move would be to const the args now. -- To view, visit https://gerrit.osmocom.org/4006 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f88c93872ff913bc211f560b26901267f577324 Gerrit-PatchSet: 13 Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: dexter <pma...@sysmocom.de> Gerrit-HasComments: Yes