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@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