[Swan] initial thoughts on uncrustifying libreswan

D. Hugh Redelmeier hugh at mimosa.com
Thu May 23 10:43:54 EEST 2013


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++;

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

- I don't much like the way parameter declarations are indented but I
  don't think it matters much.

More to come.


More information about the Swan mailing list