25 Apr
2016
25 Apr
'16
1:03 p.m.
Hi Max,
On Mon, Apr 25, 2016 at 12:22:17PM +0200, msuraev(a)sysmocom.de wrote:
* Explicitly use gsm_chan_t enum when checking for
full rate channels.
* Consistently use enums instead of (u)int, char etc.
Is this supposed to be a pure cleanup patch (not changing behavior) or a
bug fix (changing behavior)? This is currently not clear to me
I'm not particularly happy that in many cases when there is a bug to
fix, you first seem to restructure/cleanup the code and then address the
bug. This adds the risk of introducing behavioral changes in the first
step (particularly with lack of good test coverage), and it also means
we're focussing less on the relevant topic.
Note: because actual enum representation is up to
compiler this might
change the size of gsm_mncc struct so the MNCC protocol version is
bumped.
I think this kind of change just creates more effort for everyone and I
don't really see much benefit of it. You would have to merge related
changes not only to the NITB but to all its users (lcr, mncc_python,
osmo-sip-connector, ...) at the same time. Have you changed all the
respective users and re-tested them? Do you think it is a good use of
our time to do so for a merely stylistic improvement?
In general I think it is a bad idea to have enum's in structures that
describe an external interface / protocol betweeo two programs, exactly
for the reason you stated. Alignment differences can be easily avoided
with __attribute__ ((packed)), but enums have the inherent problem of
'-fshort-enums', which might be default on some platform. and then you
add a new enum value, and your structure size changes again.
Regards,
Harald
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)