5 Apr
2016
5 Apr
'16
11:18 p.m.
Hey,
Was scrolling across some of your code and saw some syntactical touching up
and optimizations that could be made. They're attached, if you'd like to
commit them.
Best,
Matthew Daiter
6 Apr
6 Apr
10:05 a.m.
Hi Matthew,
thanks for your patches and your interest in contributing.
On Tue, Apr 05, 2016 at 11:18:51PM +0200, Matthew Daiter wrote:
- if (!msc->msc_con->is_authenticated)
- continue;
- if (!is_emerg && msc->type != MSC_CON_TYPE_NORMAL)
- continue;
- if (is_emerg && !msc->allow_emerg)
+ if ((!msc->msc_con->is_authenticated) ||
+ (!is_emerg && msc->type != MSC_CON_TYPE_NORMAL) ||
+ (is_emerg && !msc->allow_emerg))
continue;
I thinkt it's a matter of taste. To me, the existing code actually is
more obvious and easier to read, as convolutd nested parenthesis in a
single 'if' statement, where there are more possible relationship
of the individual conditions.
- if (memcmp(&lai, &lu->lai, sizeof(lai))
!= 0) {
+ if (memcmp(&lai, &lu->lai, sizeof(lai))) {
again here, it is a question of taste. The '!= 0' proabably serves as
a reminder at the strange behavior of memcmp() returning 0 in case of
success.
So I'm sorry, but I wouldn't merge any of your patches, I don't think
they make the core more readable or improve it in any other way :(
I'd hope you could focus your interest in contributing into a different
area that actually makes a difference to the OpenBSC user community.
Thanks!
--
- 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)
5:40 p.m.
On Wed, Apr 06, 2016 at 10:05:46AM +0200, Harald Welte wrote:
I always do the != 0 with strcmp :)
Though I wouldn't bother to commit a patch to change it, I agree with != 0.
Thanks for joining us, Matthew! Do stay around and help us out :)
~Neels
Hi Matthew,
thanks for your patches and your interest in contributing.
On Tue, Apr 05, 2016 at 11:18:51PM +0200, Matthew Daiter wrote:
I also prefer the separate if()s for instant clarity.
- if (!msc->msc_con->is_authenticated)
- continue;
- if (!is_emerg && msc->type != MSC_CON_TYPE_NORMAL)
- continue;
- if (is_emerg && !msc->allow_emerg)
+ if ((!msc->msc_con->is_authenticated) ||
+ (!is_emerg && msc->type != MSC_CON_TYPE_NORMAL) ||
+ (is_emerg && !msc->allow_emerg))
continue;
I thinkt it's a matter of taste. To me, the existing code actually is
more obvious and easier to read - if
(memcmp(&lai, &lu->lai, sizeof(lai)) != 0) {
+ if (memcmp(&lai, &lu->lai, sizeof(lai))) {
again here, it is a question of taste. The '!= 0' proabably serves as
a reminder at the strange behavior of memcmp() returning 0 in case of
success.
3364
days inactive
3365
days old
2 comments
3 participants
participants (3)
-
Harald Welte -
Matthew Daiter -
Neels Hofmeyr