Re: Warn on macros with flow control statements

[Julia Lawall <julia.lawall@inria.fr>]

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.

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 :)

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.

julia


>
> PS: I was trying to implement your suggestion and was able to connect to
> what julia wrote here.
>
> For reference this is the link to your suggestion https://lore.kernel.org/outreachy/64154d438f0c8_28ae5229421@iweiny-mobl.notmuch/
>
> Regards,
> Sumitra
>
> > Ira
>