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

[Alison Schofield <amsfield22@gmail.com>]

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

	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.

alisons

>>>>>>>>>>>>>>>>>>> snipped a whole lot of stuff <<<<<<<<<<<<<<<<<