On Mon, 2014-03-10 at 19:29 +0200, Kalle Valo wrote:
> Joe Perches <joe@xxxxxxxxxxx> writes:
> > On Sun, 2014-03-09 at 09:10 +0200, Kalle Valo wrote:
> >> I would need help with this checkpatch warning:
> > No idea what the warning is.
>
> Hmm, maybe I'll just disable the warning in my check script.
>
> >> > --- a/drivers/net/wireless/ath/ath6kl/debug.c
> >> > +++ b/drivers/net/wireless/ath/ath6kl/debug.c
> >> > @@ -798,12 +798,10 @@ static ssize_t ath6kl_endpoint_stats_read(struct file *file,
> >> > return -ENOMEM;
> >> >
> >> > #define EPSTAT(name) \
> >> > - do { \
> >> > - len = print_endpoint_stat(target, buf, buf_len, len, \
> >> > - offsetof(struct htc_endpoint_stats, \
> >> > - name), \
> >> > - #name); \
> >> > - } while (0)
> >> > + (len = print_endpoint_stat(target, buf, buf_len, len, \
> >> > + offsetof(struct htc_endpoint_stats, \
> >> > + name), \
> >> > + #name))
> >>
> >> I wasn't quite able to figure out what is the preferred style here. I
> >> don't see how the () style is any better, but checkpatch didn't complain
> >> at least.
> >
> > No idea what the preferred style is, but
> > I'd probably change the #define to
> >
> > #define EPSTAT(name) \
> > print_endpoint_stat(target, buf, buf_len, len, \
> > offsetof(struct htc_endpoint_stats, name), \
> > #name)
> >
> > and the uses to
> >
> > len = EPSTAT(whatever);
>
> The problem with this is that using of len as an argument is hidden but
> storing the result is visible. Maybe it's better that I just disable the
> warning for me and not worry about this.
Hey, it's your code, do what you think sensible.
But, I don't think that's the right thing to do.
It _is_ a single line macro and doesn't need
a do {} while (0) wrapper. It also doesn't
need extra parentheses either.
If you don't like the macro, you could always
expand it in-place or change the macro to avoid
passing len and only using the return value as
a +=.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
