Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cleanups - formatting only

[Andy Whitcroft <apw@shadowen.org>]

On Tue, Mar 25, 2008 at 05:07:35PM +0100, Jörn Engel wrote:
> On Tue, 25 March 2008 14:45:56 +0100, Ingo Molnar wrote:
> > 
> > and let me give an example with the your very own code that you wrote 
> > and maintain, drivers/mtd/devices/block2mtd.c:
> > 
> >                                     errors   lines of code   errors/KLOC
> > drivers/mtd/devices/block2mtd.c         10             490          20.4
> > 
> > that's pretty OK code, but not perfect, the 10 errors are:
> > 
> >  ERROR: do not use C99 // comments
> >  ERROR: need spaces around that '=' (ctx:VxV)
> >  ERROR: need spaces around that '<' (ctx:VxV)
> >  ERROR: do not use C99 // comments
> >  ERROR: do not use C99 // comments
> >  ERROR: do not use C99 // comments
> >  ERROR: do not use C99 // comments
> >  ERROR: do not use C99 // comments
> >  ERROR: do not use C99 // comments
> >  ERROR: do not initialise statics to 0 or NULL
> 
> The last should and will be fixed.  The // I don't really care about.
> Send a patch if you do.
> 
> Going over my logfs patch, I found several things that are either false
> positives or rather questionable in my book.  <adds Andy to Cc:>
> 
> 
> (foo*) should be (foo *)
> 	What does that extra space gain us?

It really gains us nothing, however that is not really the point.
The point is that consistancy is good, with the space is the more normal
'C' usage, without for 'C++'; something to do with the implication that
(foo *) is a pointer to a foo (separate things), and (foo*) is a thing
of type pointer to foo (one thing) which is more object oriented.

The "norm" is with and so it makes sense to maintain it that way.  A lot
of the layout and style choises are arbitrary, and disliked by many of us,
but we follow the style to maintain that common feel.

> ERROR: no space before that close parenthesis ')'
> #2565: FILE: fs/logfs/gc.c:294:
> +                       seg_ofs + sizeof(oh) < super->s_segsize; ) {
> 
> Actual code is this:
> 	for (seg_ofs = LOGFS_SEGMENT_HEADERSIZE;
> 			seg_ofs + sizeof(oh) < super->s_segsize; ) {
> The for() loop is missing one of its three terms.  I assume this is one
> of the effects of not having a perfect C parser.
> 

Yes that is a false positive.  I'll have a look at fixing it.

> ERROR: trailing statements should be on next line
> #5000: FILE: fs/logfs/readwrite.c:203:
> +       } else while (unlikely(TestSetPageLocked(page))) {
> 
> We have an explicit exception for "else if".  "else while" makes imo
> just as much (or as little) sense.

"else if" is at least creating an additional arm of the same control
structure.  else while is mixing two different paradigms.

> ERROR: need space after that ',' (ctx:VxV)
> #5801: FILE: fs/logfs/readwrite.c:1004:
> +               ret = logfs_segment_read(inode, ipage, this_wc->ofs, bix,level);
>                                       
> One of those examples where a missing space is the lesser of two or
> three evils.

Because to add the space would mean breaking the line to avoid exceeding
80 characters?

> ERROR: need consistent spacing around '|' (ctx:WxV)
> #8293: FILE: fs/logfs/dev_mtd.c:376:
> +                       |SLAB_MEM_SPREAD|SLAB_PANIC),

That is a false positive triggered by the '    |S' at the start of
the line.  This one is fixed in the head of my tree.  The more normal
form would be for that leading | to be on the end of the previous line,
and the exception for that was already there.

> I have no idea what this is about.  Original code:
> 	mtd_cache = kmem_cache_create("mtd_cache", sizeof(struct mtd_inode), 0,
> 			(SLAB_HWCACHE_ALIGN|SLAB_RECLAIM_ACCOUNT
> 			 |SLAB_MEM_SPREAD|SLAB_PANIC),
> 

-apw