[Swan] initial thoughts on uncrustifying libreswan

[Richard Guy Briggs]

On Thu, May 23, 2013 at 10:28:22AM -0400, Lennart Sorensen wrote:
> On Thu, May 23, 2013 at 03:43:54AM -0400, D. Hugh Redelmeier wrote:
> > Something like this is a really good idea.  The current source is a ragged 
> > mess.
> > 
> > A radical suggestion is that all code checked into libreswan should be 
> > uncrustified first.  This would ensure that the formatting was always 
> > consistent.  I strongly support this.
> > 
> > == idempotence ==
> > 
> > Unfortunately, uncrustify doesn't seem to be idempotent:
> > 	uncrustify(libreswan) != uncrustify(uncrustify(libreswan))
> > 
> > This concerns me.  It puts in doubt the idea of always uncrustifying.
> > 
> > I wonder if uncrustify reaches a fixed point, or if the source changes 
> > with each application.  Is the format getting better each time?  I doubt 
> > it.
> > 
> > 
> > == mangling macro arguments containing IF ==
> > 
> > Kim found that certain calls of the DBG macro, ones where the argument 
> > contains an if statement, confuse uncrustify.  To work around this, he
> > patched the source to eliminate these statements, uncrustified, and
> > then put them back.  This is only good as a manual process -- the
> > temporary patches are fragile.
> > 
> > I'm testing a source code change that avoids the problem and doesn't
> > make the source code too much uglier.
> > 
> > 
> > == Stylistic changes I'd prefer ==
> > 
> > - when an expression is broken by an newline, normally at a binary
> >   operator, put the operator at the start of the new line.
> > 
> >   This makes the structure of the program evident by scanning
> >   only indentation and the leftmost token on a line.

This is an unusual convention, but it completely makes sense to me and I am
willing to support it.  It doesn't look so odd for logical and/or, but it does
look weird for the comma operator.

> >   In the case of control expressions for IF, WHILE, etc.
> >   the new line should be indented the same as the start of the
> >   statement.
> > 
> > 	if (blah blah blah
> > 	&& yack yack yack)
> > 	{
> > 		/* open brace is on its own line in this case */
> > 	}

This is one of the few places I diverge with DHR.  I strongly prefer the
starting brace on the same line, with the exception of top level braces in a
file (funciton, struct and global definitions).

> > - I prefer a blank line after "break" or "return" not immediately
> >   followed by closing brace.  This highlights the control-flow
> >   exception.
> > 
> >   	if (ugh != NULL)
> > 		return ugh;
> > 
> > 	/* all seems well */
> > 
> > - if an IF, WHILE, etc body is long enough to be broken, it should
> >   probably be braced.
> > 
> >   Comment lines too mean braces should be used:
> > 
> > 	if (silly)
> > 		/* should be braced */
> > 		silly++;
> 
> I think it should always be braced.
> 
> Reason is:
> 
> When debuging, often you might do:
> 
> if (silly)
> 	silly++;
> 
> And turn it into:
> 
> if (silly)
> 	fprintf(stderr,"Silly was set to %d\n", silly);
> 	silly++;
> 
> And oops, the code is now broken because I forgot to add braces, which
> is really annoying and hard to spot visually.
> 
> > - if one side of an IF-ELSE has a brace, probably they both should.
> >   A funny worst-case example is programs/pluto/connections.c line 904.
> 
> Simple to solve if everything always has braces.
> 
> > - I suspect we should prevent uncrustify removing braces, even if the
> >   transformation is correct.  They were probably placed there for a
> >   readability reason.
> > 
> > - I don't much like the way parameter declarations are indented but I
> >   don't think it matters much.
> > 
> > More to come.
> 
> I also hate opening braces being on their own line.  Waste of screen
> space. :)

I'm with Len on this.

> -- 
> Len Sorensen

	slainte mhath, RGB

--
Richard Guy Briggs               --  ~\    -- ~\            <hpv.tricolour.net>
<www.TriColour.net>                --  \___   o \@       @       Ride yer bike!
Ottawa, ON, CANADA                  --  Lo_>__M__\\/\%__\\/\%
Vote! -- <greenparty.ca>_____GTVS6#790__(*)__(*)________(*)(*)_________________