On Sat, 25 Jun 2011 19:27:01 +0200 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@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@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 ;)
[…]