[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/1] include: Auto-generate the
Re: [Qemu-block] [Qemu-devel] [PATCH v2 1/1] include: Auto-generate the sizes lookup table
Thu, 10 Jan 2019 09:42:44 +0000
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
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.
Re: [Qemu-block] [PATCH v2 1/1] include: Auto-generate the sizes lookup table, Kevin Wolf, 2019/01/08