Eric Blake <ebl...@redhat.com> 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 <ben...@irqsave.net> >> --- > >> 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 <ebl...@redhat.com>