On Wed, 11 Mar 2015, Jeff Haran wrote:
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of
> [email protected]
> Sent: Wednesday, March 11, 2015 10:00 AM
> To: Nicholas Mc Guire
> Cc: Bj??rn Mork; [email protected]
> 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
[email protected]
http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
