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

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

On Mon, 3 Jun 2013, Ondrej Bilka wrote:

> > The indentation was correct formatting for K&R C.
> > If you change anything at all, move to ISO C:
> > 
> > void
> > _IO_new_file_init (struct _IO_FILE_plus *fp)
> > {
> > 
> I updated my formatter to handle this and other. Here is v2

I don't think the patch should be doing so many different things.  It's 
more than reformatting (so your ChangeLog entry is misleading); you're 
changing function definitions to ISO C format, and using __glibc_unlikely, 
as well.  I think a patch should do one thing and do it well.  So separate 
pure formatting changes (changes that don't affect the sequence of 
preprocessing tokens at all, just whitespace and maybe comment formatting) 
from conversion of function definitions to ISO C format, from use of 
__glibc_likely / __glibc_unlikely, from removal of "register".  Each such 
cleanup is generally appropriate, but should be separated from different 
cleanups to make it as clear as possible what the patch does.

> -#define CLOSED_FILEBUF_FLAGS \
> -  (_IO_IS_FILEBUF+_IO_NO_READS+_IO_NO_WRITES+_IO_TIED_PUT_GET)
> +#define CLOSED_FILEBUF_FLAGS                                                  \
> +  (_IO_IS_FILEBUF + _IO_NO_READS + _IO_NO_WRITES + _IO_TIED_PUT_GET)

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

> -	 required by any standard.  My recollection is that
> -	 traditional Unix systems did this for stdout.  stderr better
> -	 not be line buffered.  So we do just that here
> -	 explicitly.  --drepper */
> +         required by any standard.  My recollection is that
> +         traditional Unix systems did this for stdout.  stderr better
> +         not be line buffered.  So we do just that here
> +         explicitly.  --drepper */

Still wrongly replacing TABs by spaces.

> @@ -600,7 +577,7 @@ _IO_new_file_underflow (fp)
>    fp->_IO_read_base = fp->_IO_read_ptr = fp->_IO_buf_base;
>    fp->_IO_read_end = fp->_IO_buf_base;
>    fp->_IO_write_base = fp->_IO_write_ptr = fp->_IO_write_end
> -    = fp->_IO_buf_base;
> +					     = fp->_IO_buf_base;

No, two-column indent is what Emacs does here.

> @@ -706,7 +682,7 @@ mmap_remap_check (_IO_FILE *fp)
>        /* Life is no longer good for mmap.  Punt it.  */
>        (void) __munmap (fp->_IO_buf_base,
>  		       fp->_IO_buf_end - fp->_IO_buf_base);
> -    punt:
> +punt:

No, the existing label indentation was correct and matches Emacs; labels 
(and, generally, anything not starting the definition of a function or 
another toplevel entity) should not go in the first column as that 
confuses "diff -p" and such tools (which means a "resync:" label in column 
1 later in this file is wrong, and should go in column 2).

> @@ -894,7 +868,7 @@ _IO_new_file_sync (fp)
>  	fp->_IO_read_end = fp->_IO_read_ptr;
>  #ifdef ESPIPE
>        else if (errno == ESPIPE)
> -	; /* 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.

> @@ -1452,22 +1394,20 @@ _IO_file_xsgetn (fp, data, n)
>  }
>  libc_hidden_def (_IO_file_xsgetn)
>  
> -static _IO_size_t _IO_file_xsgetn_mmap (_IO_FILE *, void *, _IO_size_t);
>  static _IO_size_t
> -_IO_file_xsgetn_mmap (fp, data, n)
> -     _IO_FILE *fp;
> -     void *data;
> -     _IO_size_t n;
> +_IO_file_xsgetn_mmap (_IO_FILE *, void *, _IO_size_t);

No, that's wrong; function prototypes that are not definitions should go 
on one line (or if long, be wrapped inside the ()), not have the 
declaration specifiers on a separate line as for definitions.  But 
actually in this case the conversion to a prototype definition would allow 
removal of the forward declaration.

-- 
Joseph S. Myers
joseph@codesourcery.com