On 02.05.2016 16:14, Pekka Paalanen wrote: > On Mon, 2 May 2016 12:27:50 +0200 > Armin Krezović <[email protected]> wrote: > >> This adds a command line argument to print wayland-scanner version. >> >> It also makes wayland-scanner emit a comment with wayland library >> version to every file it generates. >> >> Signed-off-by: Armin Krezović <[email protected]> >> --- >> src/scanner.c | 26 ++++++++++++++++++++++++-- >> 1 file changed, 24 insertions(+), 2 deletions(-) > > Hi Armin, > > this is nice. Some tiny details below, otherwise looking good. > >> >> diff --git a/src/scanner.c b/src/scanner.c >> index 52c07a6..93c4204 100644 >> --- a/src/scanner.c >> +++ b/src/scanner.c >> @@ -26,6 +26,7 @@ >> */ >> >> #include "config.h" >> +#include "wayland-version.h" >> >> #include <stdbool.h> >> #include <stdio.h> >> @@ -64,12 +65,21 @@ usage(int ret) >> "headers, server headers, or protocol marshalling >> code.\n\n"); >> fprintf(stderr, "options:\n"); >> fprintf(stderr, " -h, --help display this help and >> exit.\n" >> + " -v, --version print the wayland >> library version that\n" >> + " the scanner was built >> against.\n" >> " -c, --include-core-only include the core >> version of the headers,\n" >> " that is e.g. >> wayland-client-core.h instead\n" >> " of >> wayland-client.h.\n"); >> exit(ret); >> } >> >> +static int >> +scanner_version(int ret) >> +{ >> + fprintf(stderr, "wayland-scanner %s\n", WAYLAND_VERSION); >> + exit(ret); >> +} > > Usage may be used with error-exit, but this never, so the argument is > not necessary. However, the EXIT_SUCCESS in the call is a strong > suggestion that this call will exit, so it's good for documentation. > Well, that's all just cosmetics. Fine as is, too. >
See below (##).
>> +
>> static bool
>> is_dtd_valid(FILE *input, const char *filename)
>> {
>> @@ -1420,6 +1430,8 @@ emit_header(struct protocol *protocol, enum side side)
>> const char *s = (side == SERVER) ? "SERVER" : "CLIENT";
>> char **p, *prev;
>>
>> + printf("/* Generated by wayland-scanner %s */\n\n", WAYLAND_VERSION);
>> +
>> printf("#ifndef %s_%s_PROTOCOL_H\n"
>> "#define %s_%s_PROTOCOL_H\n"
>> "\n"
>> @@ -1621,6 +1633,8 @@ emit_code(struct protocol *protocol)
>> struct wl_array types;
>> char **p, *prev;
>>
>> + printf("/* Generated by wayland-scanner %s */\n\n", WAYLAND_VERSION);
>> +
>> if (protocol->copyright)
>> format_text_to_comment(protocol->copyright, true);
>>
>> @@ -1698,7 +1712,7 @@ int main(int argc, char *argv[])
>> char *input_filename = NULL;
>> int len;
>> void *buf;
>> - bool help = false, core_headers = false;
>> + bool help = false, core_headers = false, version = false;
>
> I'd prefer a whole new statement.
>
>> bool fail = false;
>> int opt;
>> enum {
>> @@ -1710,11 +1724,12 @@ int main(int argc, char *argv[])
>> static const struct option options[] = {
>> { "help", no_argument, NULL, 'h' },
>> { "include-core-only", no_argument, NULL, 'c' },
>> + { "version", no_argument, NULL, 'v' },
>> { 0, 0, NULL, 0 }
>> };
>>
>> while (1) {
>> - opt = getopt_long(argc, argv, "hc", options, NULL);
>> + opt = getopt_long(argc, argv, "hvc", options, NULL);
>>
>> if (opt == -1)
>> break;
>> @@ -1723,6 +1738,9 @@ int main(int argc, char *argv[])
>> case 'h':
>> help = true;
>> break;
>> + case 'v':
>> + version = true;
>> + break;
>> case 'c':
>> core_headers = true;
>> break;
>> @@ -1737,10 +1755,14 @@ int main(int argc, char *argv[])
>>
>> if (help)
>> usage(EXIT_SUCCESS);
>> + else if (version)
>> + scanner_version(EXIT_SUCCESS);
>
> Ok.
>
(##) So, am I going to leave this as it is to be consistent or change it
as you suggested above?
>> else if ((argc != 1 && argc != 3) || fail)
>> usage(EXIT_FAILURE);
>> else if (strcmp(argv[0], "help") == 0)
>> usage(EXIT_SUCCESS);
>> + else if (strcmp(argv[0], "version") == 0)
>> + scanner_version(EXIT_SUCCESS);
>
> This is not necessary. I think it is enough if we have 'wayland-scanner
> -v' and 'wayland-scanner --version' working, we don't need
> 'wayland-scanner version' too.
>
>> else if (strcmp(argv[0], "client-header") == 0)
>> mode = CLIENT_HEADER;
>> else if (strcmp(argv[0], "server-header") == 0)
>
> So why do we have this old style arguments here? I suppose because they
> are not modifiers per se, but one of them is mandatory on each
> invocation to generate anything.
>
>
> Thanks,
> pq
>
The rest of the nitpicks by You and Bryce will be taken care of. Thanks
for the review.
Armin
signature.asc
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
