Re: [Qemu-devel] [PATCH 1/1] include: Auto-generate the sizes lookup table

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]

On 1/3/19 3:21 PM, Leonid Bloch wrote:
> Hi,
> 
> On 1/4/19 12:04 AM, Eric Blake wrote:
>> On 1/3/19 2:57 PM, Leonid Bloch wrote:
>>
>>>> I have to say that I'm not very convinced of the benefits of replacing a
>>>> set of trivial numeric macros with a longer and harder to read shell
>>>> script accompanied by changes to the build system.
>>>
>>> I think that the benefit is that the script is easily verifiable,
>>> whereas if someone would like to verify the table, they will need to
>>> generate it themselves. Also, this table is automatically generated
>>> anyway, so it only makes sense to generate it during the build.
>>
>> But no one is submitting patches to active modify the table.  The work
>> has already been done once to generate it, and the reviewers have
>> already verified it; at this point, no further changes are likely to
>> happen (other than my pipe dream of entirely getting rid of the table in
>> favor of using runtime generation of human-friendly default strings is
>> added to QemuOpts instead).  If the table were not already in git, then
>> generating it at build time might make sense; but at this stage in the
>> game, you're slowing down every build to regenerate something that is
>> already correct.
>>
> 
> OK, that's a good point. But still, I think that you agree that it would 
> be more correct to generate it during the build, right? So now it is 
> there already and it works. But isn't it worth it to make it more correct?

I said "might make sense", not "I would have done it that way".  I also
don't see how a generated table is "more correct" than a hand-written
one - they are both equally correct if all values in the table match
what they are documented to provide.

In my view, code generators make sense when used on code that is
expected to change over time (a good example is QAPI because we add new
commands every release; other places might be a generator to help deal
with syscall handlers since newer kernels can add a syscall; or even the
fact that we have used some powerful GNU make textual processing to make
it easy to add files to particular subsets of the build with as few
lines edited as possible), where the goal is that the generator gives
you both a compact representation that is easier to edit, and that the
expansion from the generator ensures that repetitive boilerplate is
formed without typos.  In short, if a generator results in a net
reduction in lines of edited source in relation to the lines it
produces, AND if the source that gets regenerated is likely to change,
then it makes total sense to spend time on the generator.  But when the
amount of effort to write a generator costs as much as just hard-coding
the list outright, especially when the list is not going to change
(there really aren't any other powers of 2 within 64 bits), I'm not sure
a generator adds any value.  Unfortunately, your patch diffstat of:

>  .gitignore           |  1 +
>  Makefile             |  5 +++
>  block/qcow2.h        |  2 +-
>  block/vdi.c          |  1 +
>  include/qemu/units.h | 73 --------------------------------------------
>  scripts/gen-sizes.sh | 66 +++++++++++++++++++++++++++++++++++++++
>  6 files changed, 74 insertions(+), 74 deletions(-)
>  create mode 100755 scripts/gen-sizes.sh

is a wash (you did not result in any fewer lines in the codebase
overall), and the commit message did not do a good job saying WHY we
need it (you said WHAT it does - avoiding a hard-coded list - but not
WHY the hardcoded list is so bad that a generator would be better).  So
I'm not seeing the point of this patch.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]