Attention is currently required from: laforge, lynxis lazus, pespin.
Patch set 5:Code-Review +1
14 comments:
Patchset:
just some cosmetic and maintainability comments.
File src/libvlr/vlr_lu_fsm.c:
about the doxygen format comment:
the `/*!` is in the wrong place here.
To be associated with the 'N' member, a `/*! ... */` doxygen comment needs to go above the item.
For inline commenting, you need to use `/*< ... */` instead.
(no need to use doxygen IMHO, and IMHO comments above are in general nicer than inline ones, less clutter in future editing.)
Patch Set #5, Line 569: static void vlr_lu_compl_fsm_reset_n(struct osmo_fsm_inst *fi, uint32_t prev_state)
I'll copy the same code in here then. […]
I think this does not need to change -- this function is called for multiple different FSM onenter events, so there is no single accurate name.
So I think the naming is actually chosen well.
We could have three individually named my_state_name_onenter() functions and each of them calls the non-duplicated vlr_lu_compl_fsm_reset_n() function. That would be ultra perfect but given the short function body, this is clearly clutter, which is also a thing to aviod.
All in all this is an even steven, on a purely cosmetic item, and i would just leave the patch as it is.
Patch Set #5, Line 587: return 1;
(personally i prefer incrementing on a separate code line, for more trivial code clarity)
Patch Set #5, Line 589: LOGPFSML(fi, LOGL_ERROR, "LU Compl timeout %d / %d\n", fi->T, lcvp->N);
let's have "T%d / N%d"
Patch Set #5, Line 598: break;
(we often have a default: OSMO_ASSERT(false) in these to ensure we notice enum bugs)
Patch Set #5, Line 771: /*! count times timer T timed out */
(maybe: "count nr of times that timer...")
Patch Set #5, Line 1437: static void lu_fsm_reset_n(struct osmo_fsm_inst *fi, uint32_t prev_state)
Acknowledged
i agree with pau here because it is only one FSM state's onenter action
(would be nice to have a short one-liner comment to indicate what this does / why it needs to go in the rather unusual _preterm())
a libosmocore typo?
Patch Set #5, Line 1586: gsm48_cause = GSM48_REJECT_NETWORK_FAILURE;
(add "break;" in the end)
typo
Patch Set #5, Line 1611: fi->T
technically, middle arg should be 0 or -1 or so, because vlr_is_cs() == false from above.
(btw, i wonder a bit why it is necessary to pass two args to vlr_timer_secs(), still waiting for the caller that passes two distinct values. the patch adding the second arg is already merged.)
(two blank lines, unusual for osmocom)
To view, visit change 38490. To unsubscribe, or for help writing mail filters, visit settings.