[Swan] initial thoughts on uncrustifying libreswan

[Philippe Vouters]

I agree the Linux kernel looks well written. However from an outside 
point of view as a code reader, the Linux kernel source reading has been 
of strictly no help for me to understand and correct the kernel oops my 
users and I faced and that I describe at 
http://vouters.dyndns.org/tima/Linux-drivers-Troubleshooting_a_oops.html. In 
this case and for me there has been lots of intuition deployed with 
absolutely no certainty.

I much prefer reading Libreswan code with its numerous DBG_logs rather 
than the Linux kernel code. Thery make follow the story far more easy.

Philippe Vouters (Fontainebleau/France)
URL: http://vouters.dyndns.org/
SIP: sip:Vouters at sip.linphone.org

On 05/23/2013 12:35 PM, David McCullough wrote:
> D. Hugh Redelmeier wrote the following:
>> 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.
> Using a tool to get started sounds like a good idea, followed by
> hand tweaking and rejection of poorly formated changes from there.
> I don't it needs to be run continuously.
>
>> == 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 ==
> Agree with almost everything said here.
>
> My preference is the Linux kernel style whenever possible.
>
>> - 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 don't like this personally.
>
>> - 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++;
>>
>> - 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.
>>
>> - I suspect we should prevent uncrustify removing braces, even if the
>>    transformation is correct.  They were probably placed there for a
>>    readability reason.
> Agree with these.
>
>> - I don't much like the way parameter declarations are indented but I
>>    don't think it matters much.
> Again,  my preference is the Linux kernel style whenever possible.
> Hopefully we can avoid another style ;-)
>
> Cheers,
> Davidm
>