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

[Leonid Bloch <lbloch@janustech.com>]

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.