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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20080326102654.GB22847@logfs.org>
Date:	Wed, 26 Mar 2008 11:26:54 +0100
From:	Jörn Engel <joern@...fs.org>
To:	Andy Whitcroft <apw@...dowen.org>
Cc:	Ingo Molnar <mingo@...e.hu>, David Miller <davem@...emloft.net>,
	jirislaby@...il.com, viro@...IV.linux.org.uk, joe@...ches.com,
	tglx@...utronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 109/148] include/asm-x86/serial.h: checkpatch cleanups - formatting only

On Wed, 26 March 2008 09:52:49 +0000, Andy Whitcroft wrote:
> On Tue, Mar 25, 2008 at 05:07:35PM +0100, Jörn Engel wrote:
> > 
> > (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.

Then I'll happily ignore it.  Not having the space gains me one column.
It is absolutely minimal, sure.  But when the alternative is based on
pure whim...

> > 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.

Fairly weak grounds to argue on.  Not that mine are much stronger, I
just default to less (shorter, fewer indentations, etc.) when lacking a
reason to use more (characters, lines, indentations, etc.).

> > 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?

Or breaking the line.  Either of those choices sucks.  Well, breaking
the line is often the lesser of those evils, but in this particular
function it looks worse to me - and I have to stare at it often enough
to care.

The best strategy usually is to rethink the code and reduce the
indentation, number of arguments or length of identifiers.  I just don't
see a good way of doing that without resorting to
		ret = logfs_segment_read(i, p, w->o, b, l);

Probably nothing checkpatch should worry about.  Although I would have
been happy to have finer-grained options to enable/disable particular
warnings on the command line.  Right now I commented out several lines
in checkpatch.pl.

Jörn

-- 
Joern's library part 7:
http://www.usenix.org/publications/library/proceedings/neworl/full_papers/mckusick.a
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/