Yes, it's just rewrite of a cleanup from before which was not accepted
because it had booleans in function signature so it's safe to just drop.
The actual fix for a bug is in separate patch which does not depend on
this one.
Before fixing a bug I've got to first understand what existing code does
which is not always immediately clear, especially if comments are
missing. While going over a code I sometimes change/simplify functions
more as a "note to self" to make sure I easier understand particular
part of the code when getting back to it. If I feel that such changes
might be generally useful than I send it as a patch. Of course such
feeling is not always 100% match with what others feel so not all the
patches go through.
On 04/25/2016 03:03 PM, Harald Welte wrote:
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
--
Max Suraev <msuraev(a)sysmocom.de>
http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte