Re: [Outreachy kernel] [PATCH 1/3] staging: wilc1000: wilc_spi: Rearrange the code

[Alison Schofield <amsfield22@gmail.com>]

On Thu, Mar 09, 2017 at 10:13:58PM +0100, Julia Lawall wrote:
> 
> 
> On Thu, 9 Mar 2017, Alison Schofield wrote:
> 
> > On Thu, Mar 09, 2017 at 09:27:30PM +0100, Julia Lawall wrote:
> > >
> > >
> > > On Thu, 9 Mar 2017, Alison Schofield wrote:
> > >
> > > > On Thu, Mar 09, 2017 at 08:24:28AM +0000, Tamara Diaconita wrote:
> > > > > Hi,
> > > > > The problem with the code was when I tried to split the lines with
> > > > > dev_err(...). If I split them, I get errors like "missing terminating "
> > > > >  character" even though this character is on the next line.
> > > > > But if I don't split the line, I get warnings from checkpatch.pl.
> > > > > Is it ok if I ignore these warnings?
> > > >
> > > > Tamara,
> > > >
> > > > I looked at func wilc_spi_read_int() with the intent of changing the line
> > > > over 80 warning on dev_err to see your issue, but..the function is quite
> > > > messy.  There is excessive bracketing {} and indentation going on.
> > > >
> > > > I'm thinking we need to clean up the excessive bracketing and indentation
> > > > first, and then see which lines remain over 80 and deal with them.
> > > >
> > > > Julia - Can you take a look at this func and tell us if you agree that the
> > > > extra brackets are not a desired style?
> > >
> > > Definitely agree.  It's OK to declare variable at eg the top of an if
> > > branch if that seems really necessary from a conceptual point of view.
> > > But they shouldn't be declared at all random places, as is done in this
> > > function.  It's fine to just move the variables to the top of the
> > > function.
> > >
> > > Kernel goto labels also don't have _ around them.
> > >
> > > julia
> > >
> >
> > Tamara,
> >
> > How about a patchset that only focuses on this function:
> >
> >  Subject: [PATCH 0/N] staging: wilc1000: clean up wilc_spi_int.c

Agreed w Julia about focusing, but then wrote the file name instead
of the function name. Should be: clean up function wilc_spi_read_int()
alisons

> >
> > 	where N is yet to be defined.
> >
> >   Let's assume patches 1 through N will each depend on the previous.
> >   I'm thinking this might be a smooth way to do it.
> >   See what you think and adjust as works best for you.
> >
> >   patch-1: declare variables at top of function
> >   patch-2: remove useless brackets
> >   patch-3: use kernel preferred indentation style
> >
> >   *** see what you are left with, maybe you'll have:
> >
> >   patch-4: ...something you fit with a line
> >   patch-5: ...spelling
> >
> >   Wow  - it get's big and we're only focused on one function ;)
> >
> >   Mail the patchset to yourself.  Get a clean branch and
> >   apply/compile each patch in sequence.  It should compile
> >   after each addition.
> >
> >   So, we're basically abandoning the previous patchset.
> >   That's ok.  You can get back to those other pieces in the
> >   file after this patchset gets ack'd.
> 
> As a general rule, I think it could be a good idea to try to make smaller
> patchsets.  Focus on one thing and try to do a good job of it (proper
> subject line, helpful and comprehensive commit message, correct changes)
> rather than trying to do everything to clean up a file at once.  It's
> always possible to do the rest later.
> 
> julia
> 
> >
> > alisons
> >
> > >>>>>>>>>>>>>>>>>>> snipped a whole lot of stuff <<<<<<<<<<<<<<<<<
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170309210619.GA9289%40d830.WORKGROUP.
> > For more options, visit https://groups.google.com/d/optout.
> >