Re: Warn on macros with flow control statements

[Ira Weiny <ira.weiny@intel.com>]

Julia Lawall wrote:
> 
> 
> On Wed, 22 Mar 2023, Sumitra Sharma wrote:
> 
> > On Fri, Mar 17, 2023 at 11:19:12PM -0700, Ira Weiny wrote:
> > > Julia Lawall wrote:
> > > > > Based on the flow control I would do something like this.  It is a bit more
> > > > > verbose but is very clear what is happening.
> > > > >
> > > >
> > > > Ira, what do you think of my suggestion of
> > > >
> > > > err = err || qlge_fill_seg_(...);
> > > >
> > > > It would avoid 40 lines of ifs, and would still avoid calling
> > > > qlge_fill_seg_ as soon as there is a failure.  On ther other hand, I don't
> > > > recall this being done elsewhere, so maybe all the ifs would be
> > > > preferable.
> > >
> > > Oh sorry.  I miss read your suggestion and I thought it was going to
> > > continue running qlge_fill_seg_() for all the other calls.  I read this
> > > as:
> > >
> > > err = err | qlge_fill_seg_(...);
> > >
> > > Not sure why.  Just late I guess.
> > >
> > > Yes your suggestion would work.  I do think it is a bit odd though.
> > >
> >
> > Hi Ira,
> >
> > I would like to know why do you think this is odd?
> 
> err = err || ... is not a very common pattern.  I looked through te full
> Linux kernel for them, and found a few.

I concede!  :-D

This is just not a pattern I'm used to so it looked odd to me.

> 
> You can see one in the function rtl8723_dequeue_writeport in
> staging/rtl8723bs/hal/rtl8723bs_xmit.c.  This one is completely useless,
> so it's another cleanup opportunity :)
>

Indeed.

> 
> Some others are in loops, like (scripts/dtc/checks.c):
> 
>         for (i = 0; i < c->num_prereqs; i++) {
>                 struct check *prq = c->prereq[i];
>                 error = error || run_check(prq, dti);
>                 if (prq->status != PASSED) {
>                         c->status = PREREQ;
>                         check_msg(c, dti, NULL, NULL, "Failed prerequisite '%s'",
>                                   c->prereq[i]->name);
>                 }
>         }
> 
> The complete set of files containing this pattern is:
> 
> drivers/staging/rtl8723bs/hal/rtl8723bs_xmit.c
> drivers/net/ethernet/intel/i40e/i40e_txrx.c
> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
> scripts/dtc/checks.c
> kernel/rcu/tree.c
> arch/mips/pci/pci-legacy.c
> drivers/s390/block/dasd_devmap.c
> drivers/net/ethernet/intel/iavf/iavf_txrx.c
> 
> So not many.  But in your case the err = err || ... would be a lot omre
> concise than the version that has ifs everywhere and it owuld be a little
> more understandable than the version that hides the early returns in the
> macro definition.

Yes much better I think.  Sorry for the delay in getting back to you all.
Ira