[coreboot] [PATCH] flashrom patch easier board_pciid_enables parsing
libv at skynet.be
Thu Jan 15 04:42:13 CET 2009
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.
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.
More information about the coreboot