| Message ID | 1297464103-9111-1-git-send-email-wd@denx.de |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, 11 Feb 2011 23:41:43 +0100 Wolfgang Denk <wd@denx.de> wrote: > + case 'V': > + /* > + * Skip the "U-Boot " part in > + * U_BOOT_VERSION by adding 7 > + */ > + printf("mkimage version %s\n", > + U_BOOT_VERSION + 7); I'd have done it without magic nor comments as U_BOOT_VERSION[sizeof("U-Boot ") + 1] or even U_BOOT_VERSION[strlen("U-Boot ")] Kim
Le 12/02/2011 00:11, Kim Phillips a écrit : > On Fri, 11 Feb 2011 23:41:43 +0100 > Wolfgang Denk<wd@denx.de> wrote: > >> + case 'V': >> + /* >> + * Skip the "U-Boot " part in >> + * U_BOOT_VERSION by adding 7 >> + */ >> + printf("mkimage version %s\n", >> + U_BOOT_VERSION + 7); Now I get it. :) I might argue that this is kind of a hack, and that rather than trying to prune the U_BOOT_VERSION string, one should define two macros, for instance: #define PLAIN_VERSION "whatever" #define U_BOOT_VERSION "U-Boot " PLAIN_VERSION ... which has the advantage of not affecting the existing code, or a cleaner, but more invasive, change... #define U_BOOT_VERSION "whatever" /* without "U-Boot " */ #define U_BOOT_VERSION_BANNER "U-Boot " U_BOOT_VERSION ... and use the right macro for the right need. > I'd have done it without magic nor comments as > > U_BOOT_VERSION[sizeof("U-Boot ") + 1] > > or even > > U_BOOT_VERSION[strlen("U-Boot ")] The second one calls strlen() at run-time, plus it allocates the "U-Boot " string for no justifiable reason -- assuming the first one can be compile-time evaluated by the compiler, of course. > Kim Amicalement,
Dear Albert ARIBAUD, In message <4D562D0A.9060800@free.fr> you wrote: > > I might argue that this is kind of a hack, and that rather than trying > to prune the U_BOOT_VERSION string, one should define two macros, for > instance: > > #define PLAIN_VERSION "whatever" > #define U_BOOT_VERSION "U-Boot " PLAIN_VERSION I know this works find in C, but I'm not absolutely sure if this is acceptable with all assemblers. But I get the idea... Updated patch follows. Best regards, Wolfgang Denk
Dear Kim Phillips, In message <20110211171117.b9b05b01.kim.phillips@freescale.com> you wrote: > On Fri, 11 Feb 2011 23:41:43 +0100 > Wolfgang Denk <wd@denx.de> wrote: > > > + case 'V': > > + /* > > + * Skip the "U-Boot " part in > > + * U_BOOT_VERSION by adding 7 > > + */ > > + printf("mkimage version %s\n", > > + U_BOOT_VERSION + 7); > > I'd have done it without magic nor comments as > > U_BOOT_VERSION[sizeof("U-Boot ") + 1] > > or even > > U_BOOT_VERSION[strlen("U-Boot ")] Hm... I would have cleaned up such locations in U-Boot as part of my v4 patch that introduces PLAIN_VERSION - if I had found any such code. Am I missing something? Best regards, Wolfgang Denk
On Sat, 12 Feb 2011 07:47:38 +0100 Albert ARIBAUD <albert.aribaud@free.fr> wrote: > Le 12/02/2011 00:11, Kim Phillips a écrit : > > I'd have done it without magic nor comments as > > > > U_BOOT_VERSION[sizeof("U-Boot ") + 1] > > > > or even > > > > U_BOOT_VERSION[strlen("U-Boot ")] > > The second one calls strlen() at run-time, plus it allocates the "U-Boot > " string for no justifiable reason -- assuming the first one can be > compile-time evaluated by the compiler, of course. no, the compiler evaluates the strlen at compile time and eliminates the need to allocate the "U-Boot " string in both cases. that's not to say that there aren't a couple of gaffes above: the sizeof needs a - 1 instead of a + 1 (because it includes the trailing '\0'), and both expressions need to be prepended with an '&'. So, we have something like: &U_BOOT_VERSION[sizeof("U-Boot ") - 1] or &U_BOOT_VERSION[strlen("U-Boot ")] Kim
On Sat, 12 Feb 2011 10:37:16 +0100 Wolfgang Denk <wd@denx.de> wrote: > Dear Kim Phillips, > > In message <20110211171117.b9b05b01.kim.phillips@freescale.com> you wrote: > > On Fri, 11 Feb 2011 23:41:43 +0100 > > Wolfgang Denk <wd@denx.de> wrote: > > > > > + case 'V': > > > + /* > > > + * Skip the "U-Boot " part in > > > + * U_BOOT_VERSION by adding 7 > > > + */ > > > + printf("mkimage version %s\n", > > > + U_BOOT_VERSION + 7); > > > > I'd have done it without magic nor comments as > > > > U_BOOT_VERSION[sizeof("U-Boot ") + 1] > > > > or even > > > > U_BOOT_VERSION[strlen("U-Boot ")] > > Hm... I would have cleaned up such locations in U-Boot as part of my > v4 patch that introduces PLAIN_VERSION - if I had found any such > code. Am I missing something? ok, apart from the missing & and +/- gaffes, what are you talking about? The code does exactly what the + 7 does, except it's more self-explanatory. Kim
Dear Kim Phillips, In message <20110212161715.111c368f.kim.phillips@freescale.com> you wrote: > > > > I'd have done it without magic nor comments as ... > > Hm... I would have cleaned up such locations in U-Boot as part of my > > v4 patch that introduces PLAIN_VERSION - if I had found any such > > code. Am I missing something? > > ok, apart from the missing & and +/- gaffes, what are you talking > about? The code does exactly what the + 7 does, except it's more > self-explanatory. Sorry, my fault. I've misread your "I'd have done it" as "I have done it" but did not find any such code. Please ignore me. Best regards, Wolfgang Denk
diff --git a/tools/mkimage.c b/tools/mkimage.c index f5859d7..febb536 100644 --- a/tools/mkimage.c +++ b/tools/mkimage.c @@ -23,6 +23,7 @@ #include "mkimage.h" #include <image.h> +#include <version.h> static void copy_file(int, const char *, int); static void usage(void); @@ -246,6 +247,14 @@ main (int argc, char **argv) case 'v': params.vflag++; break; + case 'V': + /* + * Skip the "U-Boot " part in + * U_BOOT_VERSION by adding 7 + */ + printf("mkimage version %s\n", + U_BOOT_VERSION + 7); + exit(EXIT_SUCCESS); case 'x': params.xflag++; break; @@ -590,6 +599,8 @@ usage () params.cmdname); fprintf (stderr, " %s [-D dtc_options] -f fit-image.its fit-image\n", params.cmdname); + fprintf (stderr, " %s -V ==> print version information and exit\n", + params.cmdname); exit (EXIT_FAILURE); }
Signed-off-by: Wolfgang Denk <wd@denx.de> --- v2: fix missing argument to printf() call. v3: explain the magic "+ 7" offset into the version string tools/mkimage.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)