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.
Hope this helps,
Tobin.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
