[Qemu-devel] [PATCH v2 0/1] include: Add a comment to explain the origin of

[Qemu-devel] [PATCH v2 0/1] include: Add a comment to explain the origin of

[Leonid Bloch]

Please see the commit message for the rationale.

Difference from v1:
* Tabs removed from the code indentation of the sample code in the
  comment, in order to pass checkpatch.

Leonid Bloch (1):
  include: Add a comment to explain the origin of sizes' lookup table

 include/qemu/units.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
2.17.1

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

[Leonid Bloch]

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 <lbloch@janustech.com>
---
 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.
+ * 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);
+ *      }
+ *  }
+ */
+
 #define S_1KiB                  1024
 #define S_2KiB                  2048
 #define S_4KiB                  4096
-- 
2.17.1

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

[Philippe Mathieu-Daudé]

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 <lbloch@janustech.com>
> ---
>   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?

> +
>   #define S_1KiB                  1024
>   #define S_2KiB                  2048
>   #define S_4KiB                  4096
> 

Thanks,

Phil.

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

[Leonid Bloch]

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 <lbloch@janustech.com>
>> ---
>>   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.
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?

A question though: if it to be generated by ./configure, where do you 
suggest to put the generated values? And also: is it OK to assume 
there's AWK (or another scripting language) on the system for generating 
these?

Thanks,

Leonid.

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 <lbloch@janustech.com>
> >> ---
> >>   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

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

[Kevin Wolf]

Am 04.11.2018 um 19:07 hat Leonid Bloch geschrieben:
> Please see the commit message for the rationale.
> 
> Difference from v1:
> * Tabs removed from the code indentation of the sample code in the
>   comment, in order to pass checkpatch.

Thanks, applied to the block branch.

Kevin