[coreboot] [patch][v2] AMD Fam10 memory controller updates

Peter Stuge peter at stuge.se
Tue Apr 8 03:37:57 CEST 2008


On Mon, Apr 07, 2008 at 03:56:33PM -0600, Marc Jones wrote:
> Peter Stuge wrote:
> >> Do you have concerns that the patch is confused by the changes
> >
> > Yes, I think it is.
> >   
> >> or are you just naking on principal?
> >
> > No, not really.
> >
> >
> > This is how I reason:
> 
> So, yes, you are naking on principal.

It was sort-of a trick question since the principle applies when I
think whitespace changes cause confusion.

I NAKed because I thought the whitespace changes were too confusing
in this patch, not because of the principle.


On Mon, Apr 07, 2008 at 05:10:34PM -0700, Stefan Reinauer wrote:
> While in theory I agree with Peter that separating whitespace is a
> lot easier to read, I don't believe this is really a practical
> approach.

Sure it is, just don't change whitespace until the code changes are
done.


> When working on code, I fix whitespace on the fly, if I do at all.

My point is that it is much better for patch readability and long
term repo quality if whitespace is not changed at all while making
changes to code.


> I never put in a separate round for just fixing whitespace because
> that's just a bogus way of spending precious time.

Me neither. I don't want to change whitespace at all. Sometimes I
will actually play around with comments and whitespace during my
thought process but I review and restore forgotten changes before
sending a patch.


> So is seperating the whitespace fixes from the rest of the code
> after working on it.

Yes, I completely agree. Improvements done after the fact indeed
waste some time but also lead to higher quality source.

Personally I don't care at all about whitespace, but I do care
that changes in whitespace is mixed with changes in code.

I am the first to admit that following any coding style will be at
least slightly annoying unless it happens to be your own style. :)


> If it happens to be viable, yes, please try to make it like that,
> but NACKing a patch for that reason is unacceptable.

Hm, I should NAK patches that I have reviewed and don't want
committed, right?


> Therefore I say Acked-by: Stefan Reinauer <stepan at coresystems.de>
> for that patch as it is now, and I hope we can keep this
> low-profile.

Patches have been committed even with NAKs in the past and I consider
that a good thing. Maybe I have just misunderstood the point of NAK?


//Peter




More information about the coreboot mailing list