On 21. Sep 2017, at 19:14, Max msuraev@sysmocom.de wrote:
Hi Max,
"API" documentation:
Comments can be helpful and it is good to explain on a high level what a function will do and what the side-effects will be. But in an application I think writing:
\param[in,out] bts Pointer to BTS struct
adds not enough value. First it is wrong as the type is not "BTS" but it is the "C" gprs_rlcmac_bts.
It would be more helpful if this sort of feedback would be provided via gerrit because there it's easy to point to particular line of code. That way we can be sure that such oversights are quickly corrected. Thank you.
https://gerrit.osmocom.org/#/c/3905/8/src/gprs_rlcmac_ts_alloc.cpp https://gerrit.osmocom.org/#/c/3906/10/src/gprs_rlcmac_ts_alloc.cpp https://gerrit.osmocom.org/#/c/3912/5/src/gprs_rlcmac_ts_alloc.cpp https://gerrit.osmocom.org/#/c/3913/6/src/gprs_rlcmac_ts_alloc.cpp https://gerrit.osmocom.org/#/c/3930/5/src/gprs_rlcmac_ts_alloc.cpp https://gerrit.osmocom.org/#/c/3934/4/src/gprs_rlcmac_ts_alloc.cpp https://gerrit.osmocom.org/#/c/3807/10/src/gprs_rlcmac_ts_alloc.cpp https://gerrit.osmocom.org/#/c/3935/4/src/gprs_rlcmac_ts_alloc.cpp
the above list might not be complete but all of them show parameter comments that do not carry a lot of value. I think it is a net loss as:
* It took you time to write it * It makes a wrong statement, especially as gprs_rlcmac_bts and BTS are different types. * It takes time to review, it takes time to go through the comments to find them.
Given the amount of these comments I think it is best to address them with an out-of gerrit discussion.
holger