[flashrom] [PATCH] eliminate magic numbers indicating maximum column sizes in print_supported_chipsets and print_supported_boards_helper

[Stefan Tauner]

On Tue, 24 May 2011 09:01:13 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 24.05.2011 02:21 schrieb Stefan Tauner:

> > after a short discussion with carldani i have removed the assert stuff and
> > do some inferior array checking in selfcheck.
> >
> > we only have _extern_
> > references to the tables to sizeof does not work. we could add a "getter"
> > for the sizes of the array or bring the tables into view to improve this...
> > but i dont think it's worth it.
> 
> IMHO assert should be avoided because it is the equivalent of crashing 
> (no cleanup, no graceful error handling). Assert is OK if memory 
> corruption or a similar problem has been detected and you just want to 
> abort as quickly as possible without any cleanup to inimize further damage.

> The tests for NULL pointers of chipset_enables etc. make a lot of sense, 
> but the size checks only make sense if you know the array size from 
> elsewhere and if you want to check that the array size matches the size 
> an array walker (loop until ->somemember==NULL) would see. That means 
> the selfcheck function would have to live in the same file as the array 
> or you use another global variable which holds sizeof(array).
> 
or making the size accessible out of scope with global functions, yes.
do we want that? i think it's ok for now.

you did not mention "flashchips". i would not think that there exists a
minimal use case for having that one empty, but just to be sure:
the application of "|| flashchips[0].vendor == NULL" is ok there?

> The code below is internal-only. Please make sure it still compiles with
> # make CONFIG_INTERNAL=no
> That usually means wrapping in #if CONFIG_INTERNAL == 1
did not think about that sorry, done.

> 
> Not your fault, but I think the \n at the beginning here is really ugly. 
> It should live in the caller.
> […]
i have removed the \n at the start of all the print_supported_*
functions and rearranged line breaks in print_supported in general a
bit.

> > -	int status;
> > +	int status; /* OK=0 and NT=1 are defines only. Beware! */
> >    
> 
> Do we want an enum instead?
i like strong types, but i dont care too much in this case.

new patch attached.

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-eliminate-magic-numbers-indicating-maximum-column-sizes-in-print_supported_chipsets-and-print_supported_boards_helper.patch
Type: text/x-patch
Size: 8608 bytes
Desc: not available
URL: <http://www.flashrom.org/pipermail/flashrom/attachments/20110525/5ed27cc9/attachment.patch>