| Message ID | 1313079580-22144-1-git-send-email-computersforpeace@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
On Thursday 11 August 2011 18:19:40 Brian Norris wrote: > The help message for mtdinfo is unnecessarily disjointed. It is split > into three strings which reuse the PROGRAM_NAME string inefficiently and > don't have a consistent style. > > This fixup should provide a cleaner look with aligned columns and > easier-to-read source code. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > --- > v3: indented help message properly > > ubi-utils/mtdinfo.c | 52 ++++++++++++++++++++++++++------------------------ > 1 files changed, 27 insertions(+), 25 deletions(-) > > diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c >[ ... ] > +static void display_help(void) > +{ > + printf( > + "%1$s version %2$s - a tool to print MTD information.\n" > + "\n" > + "Usage: %1$s <MTD node file path> [--map | -M] [--ubi-info | -u]\n" > + " %1$s --all [--ubi-info | -u]\n" > + " %1$s [--help | --version]\n" > + "\n" > + "Options:\n" > + "-u, --ubi-info print what would UBI layout be if it was put\n" > + " on this MTD device\n" > + "-M, --map print eraseblock map\n" > + "-a, --all print information about all MTD devices\n" > + " Note: `--all' may give less info per device\n" > + " than, e.g., `mtdinfo /dev/mtdX'\n" > + "-h, --help print help message\n" > + "-V, --version print program version\n" > + "\n" > + "Examples:\n" > + " %1$s /dev/mtd0 print information MTD device /dev/mtd0\n" > + " %1$s /dev/mtd0 -u print information MTD device /dev/mtd0\n" > + "\t\t\t\tand include UBI layout information\n" Any reason this line, and only this line, uses tabs (\t) for formatting whilst every other line uses spaces? (FWIW, I prefer spaces, but more importantly think there should be some consistency.) Yes, this is pedantic trivia! ;-) cheers! -blf-
On Fri, Aug 12, 2011 at 12:06 AM, Brian Foster <brian.foster@maxim-ic.com> wrote: >> + "Examples:\n" >> + " %1$s /dev/mtd0 print information MTD device /dev/mtd0\n" >> + " %1$s /dev/mtd0 -u print information MTD device /dev/mtd0\n" >> + "\t\t\t\tand include UBI layout information\n" > > Any reason this line, and only this line, > uses tabs (\t) for formatting whilst every > other line uses spaces? (FWIW, I prefer > spaces, but more importantly think there > should be some consistency.) Yes, sort of. It's used to be because we "don't know" the length of the "%1$s" strings (i.e., the PROGRAM_NAME macro, which here is just "mtdinfo"). I suppose we could either hardcode the spaces in or try to do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel) to find the length of PROGRAM_NAME. I'll see how the second option works (for a potential v4 patch?) unless someone objects. > Yes, this is pedantic trivia! ;-) Still a valid question...in fact, this was something I had meant to reconsider before sending this patch, but I forgot :) Brian
On Fri, Aug 12, 2011 at 12:37, Brian Norris wrote: > On Fri, Aug 12, 2011 at 12:06 AM, Brian Foster wrote: >>> + "Examples:\n" >>> + " %1$s /dev/mtd0 print information MTD device /dev/mtd0\n" >>> + " %1$s /dev/mtd0 -u print information MTD device /dev/mtd0\n" >>> + "\t\t\t\tand include UBI layout information\n" >> >> Any reason this line, and only this line, >> uses tabs (\t) for formatting whilst every >> other line uses spaces? (FWIW, I prefer >> spaces, but more importantly think there >> should be some consistency.) > > Yes, sort of. It's used to be because we "don't know" the length of > the "%1$s" strings (i.e., the PROGRAM_NAME macro, which here is just > "mtdinfo"). I suppose we could either hardcode the spaces in or try to > do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel) > to find the length of PROGRAM_NAME. I'll see how the second option > works (for a potential v4 patch?) unless someone objects. you could probably use %*s and then pass in "" as well as <some max spacing number - strlen(PROGRAM_NAME)>. printf("%*s\n", 80 - strlen(PROGRAM_NAME), ""); -mike
On Sat, Aug 13, 2011 at 11:10 AM, Mike Frysinger <vapier.adi@gmail.com> wrote: > On Fri, Aug 12, 2011 at 12:37, Brian Norris wrote: ... >> "mtdinfo"). I suppose we could either hardcode the spaces in or try to >> do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel) > > you could probably use %*s and then pass in "" as well as <some max > spacing number - strlen(PROGRAM_NAME)>. > > printf("%*s\n", 80 - strlen(PROGRAM_NAME), ""); Yes, that's what I meant...of course "strlen" is better than writing your own ARRAY_SIZE() when you already have the full C library :) But I'm not sure about the subtraction part. Perhaps just use the length as extra indentation...I'll send a patch to make it clear! Brian "Someday, you might find yourself accidentally using printk() instead of printf() in your user-space code. When that day comes, you can say you are a true kernel hacker." --Robert Love, Linux Kernel Development
On Mon, Aug 15, 2011 at 13:27, Brian Norris wrote: > On Sat, Aug 13, 2011 at 11:10 AM, Mike Frysinger wrote: >> On Fri, Aug 12, 2011 at 12:37, Brian Norris wrote: > ... >>> "mtdinfo"). I suppose we could either hardcode the spaces in or try to >>> do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel) >> >> you could probably use %*s and then pass in "" as well as <some max >> spacing number - strlen(PROGRAM_NAME)>. >> >> printf("%*s\n", 80 - strlen(PROGRAM_NAME), ""); > > Yes, that's what I meant...of course "strlen" is better than writing > your own ARRAY_SIZE() when you already have the full C library :) gcc optimizes strlen("constant") into a number -mike
On Tuesday 16 August 2011 06:00:57 Mike Frysinger wrote: > On Mon, Aug 15, 2011 at 13:27, Brian Norris wrote: > > On Sat, Aug 13, 2011 at 11:10 AM, Mike Frysinger wrote: > >> On Fri, Aug 12, 2011 at 12:37, Brian Norris wrote: > > ... > >>> "mtdinfo"). I suppose we could either hardcode the spaces in or try to > >>> do some sort of "ARRAY_SIZE" trickery (as found in the Linux kernel) > >> > >> you could probably use %*s and then pass in "" as well as <some max > >> spacing number - strlen(PROGRAM_NAME)>. > >> > >> printf("%*s\n", 80 - strlen(PROGRAM_NAME), ""); > > > > Yes, that's what I meant...of course "strlen" is better than writing > > your own ARRAY_SIZE() when you already have the full C library :) > > gcc optimizes strlen("constant") into a number And ‘ARRAY_SIZE(...)’, in this case, is simply ‘sizeof(...)’ (or, actually, ‘sizeof(...)-1’ to be equivalent to ‘strlen’). Albeit I won't worry about the case of strlen > 80 here, I am mildly curious what the output would be if that were indeed true...? The other thing I won't worry about is the assumption every output byte in PROGRAM_NAME is a printing glyph occupying exactly one cell. That is true (in this case) .... cheers! -blf-
On Tue, Aug 16, 2011 at 12:18 AM, Brian Foster <brian.foster@maxim-ic.com> wrote: > On Tuesday 16 August 2011 06:00:57 Mike Frysinger wrote: >> On Mon, Aug 15, 2011 at 13:27, Brian Norris wrote: >> > Yes, that's what I meant...of course "strlen" is better than writing >> > your own ARRAY_SIZE() when you already have the full C library :) >> >> gcc optimizes strlen("constant") into a number I was referring to the fact that the kernel does not provide all standard C library functions, whereas user-space does. Even so, I overlooked the fact that the kernel has a "strlen" function (linux/kernel.h) > And ‘ARRAY_SIZE(...)’, in this case, is simply ‘sizeof(...)’ > (or, actually, ‘sizeof(...)-1’ to be equivalent to ‘strlen’). > > Albeit I won't worry about the case of strlen > 80 here, > I am mildly curious what the output would be if that were > indeed true...? Mike's example is a little different than my patch that was just included in mtd-utils. I don't think my version would have a problem with strlen > 80. > The other thing I won't worry about is the assumption every > output byte in PROGRAM_NAME is a printing glyph occupying > exactly one cell. That is true (in this case) .... Not sure how often anyone needs to worry about this one...really, this particular discussion is mostly an exercise in frivolity IMO, as I don't see us changing the PROGRAM_NAME for mtdinfo any time soon, and even if we did, it's not that hard to edit some whitespace. Brian
On Tuesday 16 August 2011 19:12:38 Brian Norris wrote: > On Tue, Aug 16, 2011 at 12:18 AM, Brian Foster <brian.foster@maxim-ic.com> wrote: >[ ... ] > > The other thing I won't worry about is the assumption every > > output byte in PROGRAM_NAME is a printing glyph occupying > > exactly one cell. That is true (in this case) .... > > Not sure how often anyone needs to worry about this one... In a previous job, this issue(in several variations) was real and live. So it's always in the back of my mind.... ;-) > this particular discussion is mostly an exercise in frivolity Agreed! Many thanks for your kind and patient help. cheers, -blf-
diff --git a/ubi-utils/mtdinfo.c b/ubi-utils/mtdinfo.c index 1c3a46f..2d47bd7 100644 --- a/ubi-utils/mtdinfo.c +++ b/ubi-utils/mtdinfo.c @@ -50,28 +50,32 @@ static struct args args = { .node = NULL, }; -static const char doc[] = PROGRAM_NAME " version " VERSION - " - a tool to print MTD information."; - -static const char optionsstr[] = -"-u, --ubi-info print what would UBI layout be if it was put\n" -" on this MTD device\n" -"-M, --map print eraseblock map\n" -"-a, --all print information about all MTD devices\n" -" Note: `--all' may give less info per device\n" -" than, e.g., `mtdinfo /dev/mtdX'\n" -"-h, --help print help message\n" -"-V, --version print program version"; - -static const char usage[] = -"Usage: " PROGRAM_NAME " <MTD node file path> [--map | -M] [--ubi-info | -u]\n" -" " PROGRAM_NAME " --all [--ubi-info | -u]\n" -" " PROGRAM_NAME " [--help | --version]\n" -"\n" -"Example 1: " PROGRAM_NAME " /dev/mtd0 print information MTD device /dev/mtd0\n" -"Example 2: " PROGRAM_NAME " /dev/mtd0 -u print information MTD device /dev/mtd0\n" -"\t\t\t\t and include UBI layout information\n" -"Example 3: " PROGRAM_NAME " -a print information about all MTD devices\n"; +static void display_help(void) +{ + printf( + "%1$s version %2$s - a tool to print MTD information.\n" + "\n" + "Usage: %1$s <MTD node file path> [--map | -M] [--ubi-info | -u]\n" + " %1$s --all [--ubi-info | -u]\n" + " %1$s [--help | --version]\n" + "\n" + "Options:\n" + "-u, --ubi-info print what would UBI layout be if it was put\n" + " on this MTD device\n" + "-M, --map print eraseblock map\n" + "-a, --all print information about all MTD devices\n" + " Note: `--all' may give less info per device\n" + " than, e.g., `mtdinfo /dev/mtdX'\n" + "-h, --help print help message\n" + "-V, --version print program version\n" + "\n" + "Examples:\n" + " %1$s /dev/mtd0 print information MTD device /dev/mtd0\n" + " %1$s /dev/mtd0 -u print information MTD device /dev/mtd0\n" + "\t\t\t\tand include UBI layout information\n" + " %1$s -a print information about all MTD devices\n", + PROGRAM_NAME, VERSION); +} static const struct option long_options[] = { { .name = "ubi-info", .has_arg = 0, .flag = NULL, .val = 'u' }, @@ -105,9 +109,7 @@ static int parse_opt(int argc, char * const argv[]) break; case 'h': - printf("%s\n\n", doc); - printf("%s\n\n", usage); - printf("%s\n", optionsstr); + display_help(); exit(EXIT_SUCCESS); case 'V':
The help message for mtdinfo is unnecessarily disjointed. It is split into three strings which reuse the PROGRAM_NAME string inefficiently and don't have a consistent style. This fixup should provide a cleaner look with aligned columns and easier-to-read source code. Signed-off-by: Brian Norris <computersforpeace@gmail.com> --- v3: indented help message properly ubi-utils/mtdinfo.c | 52 ++++++++++++++++++++++++++------------------------ 1 files changed, 27 insertions(+), 25 deletions(-)