On Wed, Sep 13, 2017 at 08:58:26AM -0700, Tobin C. Harding wrote:
> On Wed, Sep 13, 2017 at 04:14:29PM +0100, Liam Ryan wrote:
> > On Wed, Sep 13, 2017 at 08:47:39AM +0200, Frans Klaver wrote:
> > > On Tue, Sep 12, 2017 at 2:40 AM, Liam Ryan <liamryandev@xxxxxxxxx> wrote:
> > > > Fix checkpath-reported unbalanced braces in the following areas
> > > >
> > > > 221: FILE: drivers/staging/rtl8712/hal_init.c:221:
> > > > 392: FILE: drivers/staging/rtl8712/os_intfs.c:392:
> > > > 363: FILE: drivers/staging/rtl8712/rtl8712_cmd.c:363:
> > > > 889: FILE: drivers/staging/rtl8712/rtl8712_recv.c:889:
> > > > 902: FILE: drivers/staging/rtl8712/rtl871x_cmd.c:902:
> > > > 84: FILE: drivers/staging/rtl8712/rtl871x_ioctl_set.c:84:
> > > > 580: FILE: drivers/staging/rtl8712/rtl871x_mlme.c:580:
> > > > 593: FILE: drivers/staging/rtl8712/usb_intf.c:593:
> > > >
> > > > Signed-off-by: Liam Ryan <liamryandev@xxxxxxxxx>
> > > > ---
> > > > This is my first patch and I have several doubts about style choices
> > > >
> > > > At line 216 of hal_init.c should opening brace follow comment instead?
> > > >
> > > > At line 577 of rtl871x_mlme.c should I bring logic to one line instead of
> > > > opening the brace on the continued line?
> > > >
> > > > At line 353 of rtl8712_cmd.c the if/else is technically only 1 line each.
> > > > Should the braces still have been added per checkpath for readability?
> > > >
> > > > drivers/staging/rtl8712/hal_init.c | 4 ++--
> > > > drivers/staging/rtl8712/os_intfs.c | 4 ++--
> > > > drivers/staging/rtl8712/rtl8712_cmd.c | 4 ++--
> > > > drivers/staging/rtl8712/rtl8712_recv.c | 4 ++--
> > > > drivers/staging/rtl8712/rtl871x_cmd.c | 3 ++-
> > > > drivers/staging/rtl8712/rtl871x_ioctl_set.c | 4 ++--
> > > > drivers/staging/rtl8712/rtl871x_mlme.c | 4 ++--
> > > > drivers/staging/rtl8712/usb_intf.c | 3 ++-
> > > > 8 files changed, 16 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/rtl8712/hal_init.c b/drivers/staging/rtl8712/hal_init.c
> > > > index c83d7eb..de832b0b5 100644
> > > > --- a/drivers/staging/rtl8712/hal_init.c
> > > > +++ b/drivers/staging/rtl8712/hal_init.c
> > > > @@ -216,9 +216,9 @@ static u8 rtl8712_dl_fw(struct _adapter *padapter)
> > > > emem_sz = fwhdr.img_SRAM_size;
> > > > do {
> > > > memset(ptx_desc, 0, TXDESC_SIZE);
> > > > - if (emem_sz > MAX_DUMP_FWSZ) /* max=48k */
> > > > + if (emem_sz > MAX_DUMP_FWSZ) { /* max=48k */
> > >
> > > I would not have the comment there in the first place. It doesn't add
> > > any new information and just messes up the style.
> > I'm happy to remove the comment, the patch was accepted so I'm not sure
> > of procedure, should it be a new patch or a revision to this patch?
> >
> > In general I don't have the knowledge to decide which comments are worth keeping in the source
> > code.
>
> While you are getting started, if you are unsure of things I tend to lean towards making the minimal
> change to improve the code. A patch will be rejected if there are obvious extra fixes that need
> doing. Review will point these out, reviewers generally don't mind doing this because that is how you
> learn. Please be sure to carefully study these suggestions and learn from them so review does not
> have to point them out again unnecessarily.
>
> > is there a rule of thumb for brace placement near inline comments such as this?
>
> Like Frans said; If there is more than one line of code (irrelevant to whether it is comments or a
> single statement over two lines) braces make the code cleaner.
Thanks Tobin, my question was re the brace placement if a comment is on the
same line as the conditional. i.e.
if(foo) /*comment*/
bar
If I understand correctly the patch I submitted will be reviewed and
I'll be told if my choice to resolve this( if (foo) { /.. ) was incorrect.
Thanks everyone for the guidance and patience thus far.
If somebody requires an action from me at this point please assume that I have
missed it and let me know.
I'm very new to all of this so please do let me know if I'm sending unnecessary
mails ( this one included ) or breaking netiquette in my replies
Thanks,
Liam
>
> Hope this helps,
> Tobin.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
