[Swan] initial thoughts on uncrustifying libreswan

[Lennart Sorensen]

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.
> 
>   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 */
> 	}
> 
> - 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. :)

-- 
Len Sorensen