[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V2 2/3] qapi: Change the qapi scripts to take th
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH V2 2/3] qapi: Change the qapi scripts to take their input as first argument. |
Date: |
Fri, 28 Mar 2014 09:27:25 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) |
Eric Blake <address@hidden> writes:
> On 03/27/2014 09:33 AM, Benoît Canet wrote:
>> This patch is here to pave the way for the JSON include directive which
>> will need to do include loop detection.
>>
>
> Would also be nice to mention that it improves the error message
> quality. While 3/3 is definitely 2.1 material, 1/3 and 2/3 could
> arguably be committed in 2.0 as bug fixes due to the improved errors
> (but it's a stretch - personally I'm fine with saving the whole series
> for 2.1, as the error messages are in the build chain only for
> developers to see, and not user-visible in the end product)
>
>> Signed-off-by: Benoit Canet <address@hidden>
>> ---
>
>> 24 files changed, 76 insertions(+), 51 deletions(-)
>
>>
>> diff --git a/Makefile b/Makefile
>> index ec74039..9bec4ff 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -236,24 +236,24 @@ gen-out-type = $(subst .,-,$(suffix $@))
>> qapi-py = $(SRC_PATH)/scripts/qapi.py $(SRC_PATH)/scripts/ordereddict.py
>>
>> qga/qapi-generated/qga-qapi-types.c qga/qapi-generated/qga-qapi-types.h :\
>> -$(SRC_PATH)/qga/qapi-schema.json $(SRC_PATH)/scripts/qapi-types.py
>> $(qapi-py)
>> - $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py
>> $(gen-out-type) -o qga/qapi-generated -p "qga-" < $<, " GEN $@")
>> +$(SRC_PATH)/scripts/qapi-types.py $(qapi-py)
>> + $(call quiet-command,$(PYTHON) $(SRC_PATH)/scripts/qapi-types.py
>> $(SRC_PATH)/qga/qapi-schema.json $(gen-out-type) -o qga/qapi-generated -p
>> "qga-" < $<, " GEN $@")
>
> While I don't mind GNU getopt's ability to reorganize options to occur
> after non-options, I'm not sure it's the smartest thing to do here.
> Either the input file should be a new option (as in '-i input -o
> output') or you should consider ordering the command line to put the
> file name after all options ('-o output input' rather than 'input -o
> output'). For that matter, I feel that named options are always more
> flexible than positional arguments.
Using positional arguments for input files of compiler-like tools is
well-established practice.
I share your preference for putting positional arguments after the
options.
> That said, it looks like this script _already_ has a positional argument
> (gen-out-type) occurring before options, so you aren't making it any
> worse.
$(gen-out-type) expands into -EXT, where EXT is $@'s extension. Took me
some staring to figure that out :)
> Therefore, if you don't respin it to take the input file name as
> an option, I can live with:
>
> Reviewed-by: Eric Blake <address@hidden>