Re: [PATCH 1/6 v3] Reformat libio files

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Wed, 5 Jun 2013, Ondrej Bilka wrote:

> I disabled cleanups and now do formating only.
> 
> These cleanups do not do formating and you cannot avoid doing formating
> before or after cleanup and in both cases its somewhat messy.

In certain cases, *local* formatting changes may be needed for a cleanup, 
e.g. reindenting subsequent lines of a call - in which case the cleanup 
patch should include just those local changes needed, regardless of the 
human/tool combination used to produce the patch.  Not global formatting 
changes unrelated to the cleanup in question.

> Just do not complain that formating of pre ISO C function definitions is
> wrong and I should rewrite them to ISO C format.

That's not my complaint.  Rather:

* Formatting changes should only be made where there is a consensus that 
the new formatting is the correct standard for glibc.

* For your pre-ISO-C function definitions, you are making a change where 
the consensus is that the change is *incorrect*.

A formatting patch should never change something correctly formatting to 
be incorrect, or change something where both before and after are correct 
absent a consensus for a change.

If parts of formatting changes or cleanups are uncontroversial and parts 
are more controversial, those should be split into separate patch 
submissions.

> > That appears to be putting the backslash further to the right than Emacs 
> > C-C C-\ does (in most cases I think of Emacs formatting as a good guide if 
> > the GNU Coding Standards don't specify some detail).
> 
> What does this do? a C-C C-\ is undefined at emacs that I installed. Is
> possible to make it standalone?

C-C C-\, in C mode, puts backslashes at ends of lines in the selected 
region.

> A program cannot guess what alternative is selected so it is better to
> stick with one.

If in doubt, you can simply avoid changing existing macros using 
backslash-newline unless actually changing the contents of the macro 
definition, and just keep the backslashes in the same columns as before 
when editing the contents of an existing such macro.

> > > -	; /* Ignore error from unseekable devices. */
> > > +	;  /* Ignore error from unseekable devices. */
> > 
> > What rule do you think there is for indentation of comments after code 
> > that is followed in glibc (or generally in the GNU system)?  I wasn't 
> > aware of one, and without consensus on one, existing code shouldn't be 
> > changed like this; formatting changes should be when code clearly deviates 
> > from the established desired style.
> > 
> Again uncrustify does this. I do not know how turn it specially off. As
> it is minor I would accept this noise.

You should aim to produce patches that do a single cleanup (all changes 
disabled by default, one cleanup enabled) in preference to doing a whole 
load of changes, some lacking consensus, and then trying to disable them 
selectively.

I don't know whether uncrustify is a suitable tool for this at all.  But 
any combination of tool and human used needs to respect consensus.

-- 
Joseph S. Myers
joseph@codesourcery.com