[PATCH wayland] Add version argument to wayland-scanner

[Armin Krezović]

On 02.05.2016 16:14, Pekka Paalanen wrote:
> On Mon,  2 May 2016 12:27:50 +0200
> Armin Krezović <krezovic.armin at gmail.com> 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ć <krezovic.armin at gmail.com>
>> ---
>>  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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160504/419ed049/attachment.sig>