Re: [Qemu-devel] [PATCH v2 1/1] include: Add a comment to explain the origin of sizes' lookup table

[Kevin Wolf]

Am 06.11.2018 um 09:56 hat Leonid Bloch geschrieben:
> Hi Phil, Hi Eric,
> 
> (Eric, for some reason you weren't CC'd to this thread - sorry.)
> 
> On 11/5/18 5:58 PM, Philippe Mathieu-Daudé wrote:
> > Hi Leonid,
> > 
> > On 4/11/18 19:07, Leonid Bloch wrote:
> >> The lookup table for power-of-two sizes was added in commit 540b8492618eb
> >> for the purpose of having convenient shortcuts for these sizes in cases
> >> when the literal number has to be present at compile time, and
> >> expressions as '(1 * KiB)' can not be used. One such case is the
> >> stringification of sizes. Beyond that, it is convenient to use these
> >> shortcuts for all power-of-two sizes, even if they don't have to be
> >> literal numbers.
> >>
> >> Despite its convenience, this table introduced 55 lines of "dumb" code,
> >> the purpose and origin of which are obscure without reading the message
> >> of the commit which introduced it. This patch fixes that by adding a
> >> comment to the code itself with a brief explanation for the reasoning
> >> behind this table. This comment includes the short AWK script that
> >> generated the table, so that anyone who's interested could make sure
> >> that the values in it are correct (otherwise these values look as if
> >> they were typed manually).
> >>
> >> Signed-off-by: Leonid Bloch <address@hidden>
> >> ---
> >>   include/qemu/units.h | 18 ++++++++++++++++++
> >>   1 file changed, 18 insertions(+)
> >>
> >> diff --git a/include/qemu/units.h b/include/qemu/units.h
> >> index 68a7758650..1c959d182e 100644
> >> --- a/include/qemu/units.h
> >> +++ b/include/qemu/units.h
> >> @@ -17,6 +17,24 @@
> >>   #define PiB     (INT64_C(1) << 50)
> >>   #define EiB     (INT64_C(1) << 60)
> >> +/*
> >> + * The following lookup table is intended to be used when a literal 
> >> string of
> >> + * the number of bytes is required (for example if it needs to be 
> >> stringified).
> >> + * It can also be used for generic shortcuts of power-of-two sizes.
> > 
> > ^ ok
> > 
> > v this part is not useful in the tree IMHO.
> > 
> >> + * This table is generated using the AWK script below:
> >> + *
> >> + *  BEGIN {
> >> + *      suffix="KMGTPE";
> >> + *      for(i=10; i<64; i++) {
> >> + *          val=2**i;
> >> + *          s=substr(suffix, int(i/10), 1);
> >> + *          n=2**(i%10);
> >> + *          pad=21-int(log(n)/log(10));
> >> + *          printf("#define S_%d%siB %*d\n", n, s, pad, val);
> >> + *      }
> >> + *  }
> >> + */
> > 
> > If it is generated and the stringified are also generated, why not 
> > generate this once via the ./configure, since it is used by machines and 
> > not by humans?
> 
> You beat me to it! I was just thinking about that last evening. Indeed 
> it's not so elegant to put generated code in a file that is intended for 
> human handling. Generating by ./configure would be prettier.

We can still do this on top (and probably only for 3.2 as we're in
freeze now and it's neither a bug fix nor a documentation improvement or
test).

> A runtime solution that interprets hard-coded expression strings, like 
> Eric suggested, can also be a possibility, but it seems more complicated 
> than just generating these at the configuration stage. Plus one gets the 
> S_* constants that can be used in many places, not only where an 
> explicit number is needed. What do you think?

I think this is not a good use of our time when we want to use QAPI in
the long run anyway.

> A question though: if it to be generated by ./configure, where do you 
> suggest to put the generated values?

We already generate a lot of files, you could check where these end up.
But I suppose it might just be the root of the build directory (unless
an include/ exists there?)

> And also: is it OK to assume there's AWK (or another scripting
> language) on the system for generating these?

git grep says that configure already calls awk, so it's probably okay.
Of course, the existing test would just disable libgcrypt instead of
completely failing, so it's still a bit different.

As for other scripting languages, configure itself is written in shell,
and we also assume that Python is available (used e.g. by the QAPI
generators). At least these two are safe, too.

Kevin