On Thu, Jan 15, 2009 at 04:16:38AM +0100, Carl-Daniel Hailfinger wrote: > On 15.01.2009 03:55, Peter Stuge wrote: > > Carl-Daniel Hailfinger wrote: > > > >> Making strange code painful to look at is good. > >> > > > > ..to ensure that noone will ever want to look at it again, so that > > bugs won't be found? Makes no sense. > > > > I consider the opposite to be true, in particular code with strange > > logic should be easy on the eyes so that anyone who has even slight > > interest could actually review it and maybe even spot a bug. > > > > It is definitely easier on the eyes if you only have to look at two > places at the same time (array and board enable function) now instead of > three places (flash.h and array and board enable function) before. > > The pain I was referring to is in the loads of NULL/0 stuff. It can be > eliminated with liberal application of grep. The real question is why > there are so many NULL/0 entries in the first place. > > > > If the code is painful $person will just laugh and go away, leaving > > bugs lingering.. > > > > Actually, there are bugs lingering in related code, but nobody bothered > to fix it. Well, that's not entirely true. I bothered to send a patch, > but iirc it was never acked. I can resend... > > Anyway, this change makes it easy to add a new board without having to > open two editor windows at the same time to find out the correct struct > ordering. And that makes it easier for new contributors who just want to > fix flashrom for their board. > > The two-line layout was really painful. It was multiline (which you seem > to dislike extremely) and it required anybody changing it to have > flash.h open at the same time (which I think is more complicated than it > should be). > I consider anything to be an improvement over the old layout. > > Regards, > Carl-Daniel
flash.h? This table is fully file internal. The struct definition is right before the table! My original code was single long line per entry. Luc Verhaegen. -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
