Re: [PATCH 0/2] AArch64 glibc port

[Carlos O'Donell <carlos_odonell at mentor dot com>]

On 10/3/2012 8:04 AM, Joseph S. Myers wrote:
> On Mon, 1 Oct 2012, Carlos O'Donell wrote:
> 
>> (2) Split up your patches.
>>
>> One huge patch is impossible to review.
>>
>> I can see about 25-30 groupings of files that relate and your patch should
>> be split up along those lines.
> 
> No, absolutely do not split it up artificially.  Patch submissions should 
> be self-contained.  The port (the new sysdeps files) should be submitted 
> as a single patch, to libc-ports only (not libc-alpha), with appropriate 
> rationale for any design choices it seems appropriate to draw attention 
> to.  One patch is much easier to review, and spot whether anything is 
> missing in it, than 25-30.
> 
> The only things to split up are any libc changes required (generally, 
> anything outside of ports/sysdeps/.../aarch64/), where each logical change 
> should be sent in its own self-contained patch submission with its own 
> self-contained rationale.
> 
> (If any bits of the port are *not needed at all* for a functional port - 
> if they are purely optimized functions - they can be omitted from the 
> submission and dealt with later afer the port is in.  But still don't send 
> multiple ports patches in the initial submission.)

I agree with your comments about splitting out libc changes, logically
orthogonal changes, and optimization changes not needed for the initial port.

While I disagree about splitting up patches, given that you are the ARM
machine maintainer and have stated your preference, please feel free to
review the current patch.

The single long patch can be just as easily reconstructed by applying all
the patches and reviewing the diff yourself while still providing a logically
split up set of emails with rationale for each patch.

I find it much easier to do a piece-meal review of each mini-patch this way
*followed* by a larger overview review afterwards based on a test build
after having applied the patches.

Personally I see no benefit to having the whole patch a single entity versus 
having it split up with individual rationale listed separately.

Cheers,
Carlos.
-- 
Carlos O'Donell
Mentor Graphics / CodeSourcery
carlos_odonell@mentor.com
carlos@codesourcery.com
+1 (613) 963 1026