Patch Set 1: Code-Review-1 (8 comments)
some too detailed review of (mostly minor) cosmetics... https://gerrit.osmocom.org/#/c/2589/1/openbsc/src/libmsc/smpp_openbsc.c File openbsc/src/libmsc/smpp_openbsc.c: Line 464: #define SMPP_TO_GSM411_MAX 4 You're defining this value but not using it the array definition... (s.b.) Line 466: struct x { (no x here please) Line 469: } smpp_to_gsm411_err_array[4] = { (continued from above) ...here. In fact though we usually define the array and later use the ARRAY_SIZE() macro to iterate its items. You can/should actually omit the 4 completely, as in struct { ... } my_name[] = { {item}, {item} ... }; for (i = 0; i < ARRAY_SIZE(my_name); i++) {...} Line 477: .smpp_status_code = ESME_RSYSERR, could omit the explicit names, just { ESME_X, GSM411_X }, ... Line 495: int *gsm411_cause) (if you're using tab and space mixed-indent, then it would be nice to line up 'uint32_t' with 'int') Line 498: for (i = 0; i < SMPP_TO_GSM411_MAX; i++) { (here) Line 502: } (wrong indent) I guess we'd normally go for an early continue instead: { if (A != B) continue; do stuff return 0; } Line 571: } (No braces needed around single line.) (No real need for 'rc', could just do: if (smpp_to_gsm411_err(..) < 0) ... ) -- To view, visit https://gerrit.osmocom.org/2589 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I61fb2d9ef4f2d2eabdc49b53d9966ad328d15e51 Gerrit-PatchSet: 1 Gerrit-Project: openbsc Gerrit-Branch: master Gerrit-Owner: Keith Whyte <ke...@rhizomatica.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Pablo Neira Ayuso <pa...@gnumonks.org> Gerrit-HasComments: Yes