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

[Leonid Bloch]

Hi,

On 1/8/19 11:31 AM, Markus Armbruster wrote:
> I'd leave it hard-coded.  Replacing a few trivial defines by an arguably
> less trivial script doesn't feel like an improvement.  In this case, it
> doesn't even save lines.

As I've said, I'm fine with that. The autogeneration sounds the right 
thing to do here, as the table is autogenertaed anyway, but indeed it is 
already there, explained, and even the script for generating it appears 
in the comments for reproducibility.

> 
> I'm not sure I'd use shell for this, but since you already wrote it and
> it works...
> 

If you've noticed, the original script was in AWK. But to be as generic 
as possible, I didn't write the generation script in AWK because even 
AWK is not guaranteed to be installed on the build system. The only 
interpreted language that is guaranteed to be there is shell (most basic 
shell) because .configure itself needs it.

Sure, the script could be prettier and shorter, but I wanted to keep it 
compatible with the most basic shell, not only Bash, and without needing 
external programs.

Eric - thanks for the comment about 'local' - I will get rid of it if we 
decide to include this patch.

> 
> Unchecked use of command-line arguments is not nice:
> 
>      $ scripts/gen-sizes.sh
>      scripts/gen-sizes.sh: line 64: : No such file or directory
>      scripts/gen-sizes.sh: line 65: : No such file or directory
>      scripts/gen-sizes.sh: line 66: : No such file or directory
> 
> You should error out if $# -ne 1.  But in such a simple script, I'd
> dispense with arguments and print to stdout.  Matter of taste.
> Rejecting $# -ne 0 is still nice then.
> 

Agree, thanks!

Leonid.
___