On Wed, 11 Mar 2015, Jeff Haran wrote: > -----Original Message----- > From: kernelnewbies-bounces@xxxxxxxxxxxxxxxxx [mailto:kernelnewbies-bounces@xxxxxxxxxxxxxxxxx] On Behalf Of Valdis.Kletnieks@xxxxxx > Sent: Wednesday, March 11, 2015 10:00 AM > To: Nicholas Mc Guire > Cc: Bj??rn Mork; kernelnewbies@xxxxxxxxxxxxxxxxx > Subject: Re: confusing code....whats the point of this construct ? > > On Wed, 11 Mar 2015 17:46:56 +0100, Nicholas Mc Guire said: > > > ret = ({ long __ret = (5*250); > > do { _cond_resched(); } while (0); > > if (!({ > > bool __cond = (({ > > >Gaak. > > > > ret = wait_event_timeout(mgr->tx_waitq, > > check_txmsg_state(mgr, txmsg), > > (4 * HZ)); > > >-EGADS - use of macro as a function violates the Principle of Least Surprise.... > > > >I have to wonder how many other places we've got bugs waiting to happen because of this.... > > I don't understand the problem here. The caller passes in a condition to be evaluated in a loop. Many times that condition is quite simple (e.g. a counter being non-zero). If it was a function the caller would have to pass in a pointer to a function that does the evaluation, as in: > > int bar; > > int foo(void) > { > return bar; > } > > ... > > wait_event_interruptible(..., foo, ...); > > Instead of the much simpler: > > wait_event_interruptible(..., bar, ...); > > That latter seems easier to understand and require fewer instructions to be generated since there is no function call overhead. > for simle conditions that is true and commonly done but for complex conditions that require locking or intermediate variables it is much more readable if packed up in a function like in e.g. see drivers/gpu/drm/drm_dp_mst_topology.c:drm_dp_mst_wait_tx_reply() static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_sideband_msg_tx *txmsg) { bool ret; mutex_lock(&mgr->qlock); ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX || txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT); mutex_unlock(&mgr->qlock); return ret; } drm_dp_mst_wait_tx_reply() <snip> ret = wait_event_timeout(mgr->tx_waitq, check_txmsg_state(mgr, txmsg), (4 * HZ)); <snip> which I find is much simpler to understand than the "inline" code in ath10k_flush(). thx! hofrat _______________________________________________ Kernelnewbies mailing list Kernelnewbies@xxxxxxxxxxxxxxxxx http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
