Re: [PATCH] staging: rtl8712: Fix unbalanced braces around else statement

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux