[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