[flashrom] [PATCH 3/4] reformat -L output and add printing of chip voltage ranges to print.c

Stefan Tauner stefan.tauner at student.tuwien.ac.at
Sat Jun 25 20:30:18 CEST 2011


On Sat, 25 Jun 2011 19:27:01 +0200
Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net> wrote:

> Am 18.06.2011 22:53 schrieb Stefan Tauner:
> > besides adding output for the voltage ranges, this patch also changes
> > various aspects of the -L output:
> > - sizes are right aligned now with a fixed length of 5
> > - test results are always shown in the same column ("PR" and " R"
> >   instead of "PR" and "R ")
> > - get rid of POS_PRINT and digits
> > - wideness reductions on the whole with a remarkable detail:
> >   vendor names are split on '/' and spread over mutliple lines while
> >   all other columns are printed on the first line.
> >
> > Signed-off-by: Stefan Tauner <stefan.tauner at student.tuwien.ac.at>
> >   
> 
> Sounds good, I'll review in detail later. A few comments, though.
> 
> > ---
> >  print.c |  149 +++++++++++++++++++++++++++++++++++++++++++--------------------
> >  1 files changed, 102 insertions(+), 47 deletions(-)
> >
> > diff --git a/print.c b/print.c
> > index 80316cb..b110ff9 100644
> > --- a/print.c
> > +++ b/print.c
> > @@ -59,27 +59,16 @@ char *flashbuses_to_text(enum chipbustype bustype)
> >  	return ret;
> >  }
> >  
> > -#define POS_PRINT(x) do { pos += strlen(x); msg_ginfo(x); } while (0)
> > -
> > -static int digits(int n)
> > -{
> > -	int i;
> > -
> > -	if (!n)
> > -		return 1;
> > -
> > -	for (i = 0; n; ++i)
> > -		n /= 10;
> > -
> > -	return i;
> > -}
> > -
> >  static void print_supported_chips(void)
> >  {
> > -	int okcol = 0, pos = 0, i, chipcount = 0;
> > +	int i, chipcount = 0;
> >  	int maxvendorlen = strlen("Vendor") + 1;
> >  	int maxchiplen = strlen("Device") + 1;
> > +	int maxtypelen = strlen("Type") + 1;
> >  	const struct flashchip *f;
> > +	const char *delim ="/";
> > +	char *tmp_ven_str;
> > +	int tmp_ven_len;
> >   
> 
> Ugly variable names.

tmpven and tmpvenlen?
else i wanna hear suggestions :)

> >  	char *tmp_bus_str;
> >  
> >  	for (f = flashchips; f->name != NULL; f++) {
> > @@ -87,12 +76,22 @@ static void print_supported_chips(void)
> >  		if (!strncmp(f->name, "unknown", 7))
> >  			continue;
> >  		chipcount++;
> > -		maxvendorlen = max(maxvendorlen, strlen(f->vendor));
> > +		tmp_ven_str = (char *)f->vendor;
> > +		do {
> > +			tmp_ven_len = strcspn(tmp_ven_str, delim);
> > +			maxvendorlen = max(maxvendorlen, tmp_ven_len);
> > +			tmp_ven_str += tmp_ven_len;
> >   
> 
> You need to add +1 to the expression above. Explanation follows.
> 
> 
> > +		} while (tmp_ven_len != 0);
> >   
> 
> Are you sure this loop does what it should?
> Consider maxvendorlen=0, vendor="a/bc". The first iteration will have
> tmp_ven_len=1, maxvendorlen=1, then tmp_ven_str="/bc". The next
> iteration will have tmp_ven_len=0, maxvendorlen=1, tmp_ven_str="/bc" and
> then the loop will stop with an incorrect value of maxvendorlen.

fixed and annotated with:
/* skip to the char following the first '/' */
thanks!

> 
> > +
> >  		maxchiplen = max(maxchiplen, strlen(f->name));
> > +		tmp_bus_str = flashbuses_to_text(f->bustype);
> > +		maxtypelen = max(maxtypelen,
> > +				 strlen(flashbuses_to_text(f->bustype)));
> >   
> 
> Oooh... that one is fun. Do we really want that, or do we just want to
> assume that maximum string length of bustype is the length of all
> bustypes at once?

using the combined length of all of them together is safe but would
lead to a very wide column.
what's your problem with this version (besides the wrong usage of
flashbuses_to_text instead of tmp_bus_str)?
it looks at the length of all buses combined in each chip. why would we
want to print more than that?

> 
> > +		free(tmp_bus_str);
> >  	}
> > -	maxvendorlen++;
> > -	maxchiplen++;
> > -	okcol = maxvendorlen + maxchiplen;
> > +	maxvendorlen += 1;
> > +	maxchiplen += 1;
> > +	maxtypelen += 1;
> >   
> 
> Is there any reason you're not using ++?

yes, because i have used += 2 in an earlier version ;)

>> […]

-- 
Kind regards/Mit freundlichen Grüßen, Stefan Tauner




More information about the flashrom mailing list