From: Derek Perrin <d.roc16@gmail.com>
To: Joe Perches <joe@perches.com>
Cc: linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drivers: firmware: edd: fixed coding style errors
Date: Mon, 23 Dec 2013 18:53:46 -0600 [thread overview]
Message-ID: <2057495.KkFWMBqotF@slave1> (raw)
In-Reply-To: <1387750419.22671.25.camel@joe-AO722>
On Sunday, December 22, 2013 02:13:39 PM Joe Perches wrote:
> On Sun, 2013-12-22 at 15:17 -0600, Derek Perrin wrote:
> > Fixed coding style errors. Spaces, tab and parenthesis errors.
>
> There's a real error in this patch.
>
> When you do whitespace only changes, please
> verify that the old and new object files produced
> are unchanged.
>
> One trivial bit too,
>
> > diff --git a/drivers/firmware/edd.c b/drivers/firmware/edd.c
>
> []
>
> > @@ -62,8 +62,8 @@ struct edd_device {
> >
> > struct edd_attribute {
> >
> > struct attribute attr;
> >
> > - ssize_t(*show) (struct edd_device * edev, char *buf);
> > - int (*test) (struct edd_device * edev);
> > + ssize_t(*show) (struct edd_device *edev, char *buf);
> > + int (*test) (struct edd_device *edev);
>
> I think most prefer function pointer prototypes like:
>
> size_t (*show)(struct edd_device *edev, char *buf);
> int (*test)(struct edd_device *edev);
>
> > @@ -791,7 +791,7 @@ edd_exit(void)
> >
> > struct edd_device *edev;
> >
> > for (i = 0; i < edd_num_devices(); i++) {
> >
> > - if ((edev = edd_devices[i]))
> > + if ((edev == edd_devices[i]))
> >
> > edd_device_unregister(edev);
> >
> > }
> > kset_unregister(edd_kset);
>
> This is wrong.
> It's an assignment in the block and it's important.
>
> This could be rewritten as:
>
> for (i = 0; i < edd_num_devices(); i++) {
> edev = edd_devices[i];
> if (edev)
> edd_device_unregister(edev);
> }
>
> or just remove the temporary.
Sorry, I'm really new to contributing to the kernel and just learning the
process. I can make a new patch that fixes the assignment. I'm not seeing any
difference between what I did and what you said about the function pointer
prototypes.
next prev parent reply other threads:[~2013-12-24 0:53 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-22 21:17 [PATCH] drivers: firmware: edd: fixed coding style errors Derek Perrin
2013-12-22 22:13 ` Joe Perches
2013-12-24 0:53 ` Derek Perrin [this message]
2013-12-24 6:43 ` Joe Perches
2013-12-25 10:19 ` [PATCH v2] " Derek Perrin
2013-12-25 5:21 ` [PATCH] checkpatch: Add tests for function pointer style misuses Joe Perches
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2057495.KkFWMBqotF@slave1 \
--to=d.roc16@gmail.com \
--cc=joe@perches.com \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.