Re: confusing code....whats the point of this construct ?

March 11, 2015

      On Wed, 11 Mar 2015, Jeff Haran wrote:
...
-----Original Message-----
From: kernelnewbies-bounces@kernelnewbies.org [mailto:kernelnewbies-bounces@kernelnewbies.org] On Behalf Of Valdis.Kletnieks@vt.edu
Sent: Wednesday, March 11, 2015 10:00 AM
To: Nicholas Mc Guire
Cc: Bj??rn Mork; kernelnewbies@kernelnewbies.org
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