Message ID | CAN25KhGzs8=WcHvDGrEO-mDNKTBOia8QsW4PtF5BGN2PA=y9VQ@mail.gmail.com |
---|---|
State | Not Applicable |
Headers | show |
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!
On Wed, Apr 06, 2016 at 10:05:46AM +0200, Harald Welte wrote: > 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 I also prefer the separate if()s for instant clarity. > > - 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. 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
diff --git a/openbsc/src/osmo-bsc/osmo_bsc_filter.c b/openbsc/src/osmo-bsc/osmo_bsc_filter.c index 14e0b71..5ad9fd7 100644 --- a/openbsc/src/osmo-bsc/osmo_bsc_filter.c +++ b/openbsc/src/osmo-bsc/osmo_bsc_filter.c @@ -160,11 +160,9 @@ struct osmo_msc_data *bsc_find_msc(struct gsm_subscriber_connection *conn, round_robin: llist_for_each_entry(msc, &bsc->mscs, entry) { - 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; /* force round robin by moving it to the end */