Re: [PATCH] memset with cache line size fix.

[Geoff Keating <geoffk at geoffk dot org>]

[Note, I removed libc-alpha-owner by DJ's request, and aj, drepper,
and jakub since they'll get this mail because they're on libc-alpha,
and sjmunroe@vnet.ibm.com since I hope that's the same as
sjmunroe@us.ibm.com.  It's not necessary to send patches to individual
maintainers, just send it to the list and everyone will see it.]

> From: "Steve Munroe" <sjmunroe@us.ibm.com>
> Date: Tue, 20 Aug 2002 13:42:35 -0500

> > 3. Formatting, see Roland's comments on the more recent patches.
> 
> It is not clear to me what formatting sins I have committed. I have gone
> over the patch again and removed some tabs but I can't find anything wrong.
> Please be specific!

OK, I'll quote some sections of the coding standard, and give an
example of a place where you didn't follow it.

The standard says:
>    For the body of the function, we prefer code formatted like this:
> 
>      if (x < foo (y, z))
>        haha = bar[4] + 5;
>      else
>        {
>          while (z)
>            {
>              haha += foo (z, z);
>              z--;
>            }
>          return ++x + bar ();
>        }

But you did:
+       case AT_DCACHEBSIZE:
+               {
+                       int *cls = & __cache_line_size;
there is too much whitespace at the start of the lines.  It may be
that you have the tab settings wrong in your editor, or you may be
used to the BSD KNF style of formatting where everything is indented 8
spaces rather than 2.

The standard says:
>   Please put two spaces after the end of a sentence in your comments,
But you did:
!     /* set up cache line size etc from aux vector. */
with only one space

The standard says:
> capitalize the first word.
But you did:
+ /* define a global static that can hold the cache line size. The 
which has no 'D'.

The standard says:
> write complete sentences
But you did:
+    stuff cache line value into this variable */
which isn't complete because it doesn't have a '.'.

> > 5. This chunk is not C code; could use an explanation; and contains a
> >    Mysterious Whitespace Change.
> >
> > ***************
> > *** 75,80 ****
> >         auxvec = ubp_ev;
> >         while (*(char *__unbounded *__unbounded) auxvec != NULL)
> > !          ++auxvec;
> > !       ++auxvec;
> >  #ifndef SHARED
> >        _dl_aux_init ((ElfW(auxv_t) *) auxvec);
> > --- 97,103 ----
> >         auxvec = ubp_ev;
> >         while (*(char *__unbounded *__unbounded) auxvec != NULL)
> > !                ++(char**)auxvec;
> > !       ++(char**)auxvec;
> > !
> >  #ifndef SHARED
> >         _dl_aux_init ((ElfW(auxv_t) *) auxvec);
> 
> There is an important difference between "++auxvec" and
> "++(char**)auxvec".

You probably want to have 'auxvec' having the proper type initially,
rather than use this cast in this way.  You're aware that using a cast
like this is a GCC extension?

> (See attached file: memset225a-patch.txt)
> 
> 
> --0__=09BBE688DFF26C018f9e8a93df938690918c09BBE688DFF26C01
> Content-type: application/octet-stream; 
> 	name="memset225a-patch.txt"
> Content-Disposition: attachment; filename="memset225a-patch.txt"
> Content-transfer-encoding: base64
> 
> MjAwMi0wOC0yMCAgU3RldmVuIE11bnJvZSAgPHNqbXVucm9lQHVzLmlibS5jb20+DQoJKiBzeXNk
> ZXBzL3Bvd2VycGMvZWxmL2xpYmMtc3RhcnQuYyA6IFNjYW4gQXV4IFZlY3RvciBmb3IgDQoJQVRf

It'd be better if you could attach your patches encoded as 8bit or
7bit, not base64 (or quoted-printable).  We can cope if you don't, but
using 7bit makes it faster and easier to review patches.

(I use a non-MIME mailer to review patches, because that way when a
patch has 'Content-disposition: attachment' my mailer just ignores it
and puts the patch inline.  That way I can just reply quoting the
message, delete the bits that are OK, and add comments to the bits
that aren't.)

-- 
- Geoffrey Keating <geoffk@geoffk.org> <geoffk@redhat.com>