threads / patch / 34476

patchAdd the GIT_SENTINEL macro

Subject: [PATCH] Add the GIT_SENTINEL macro

## tl;dr

7 messages between Jul 18, 2013 and Jul 20, 2013. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

Ramsay Jones· Jul 18, 2013, 20:02 UTC · lore

The sentinel function attribute is not understood by versions of the gcc compiler prior to v4.0. At present, for earlier versions of gcc, the build issues 108 warnings related to the unknown attribute. In order to suppress the warnings, we conditionally define the GIT_SENTINEL macro to provide the sentinel attribute for gcc v4.0 and newer.

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---
This was built on the next branch

ATB, Ramsay Jones

 argv-array.h      | 2 +-
 builtin/revert.c  | 4 ++--
 exec_cmd.h        | 2 +-
 git-compat-util.h | 7 +++++++
 run-command.h     | 2 +-
 5 files changed, 12 insertions(+), 5 deletions(-)
Show changes to 5 files +12 −5

argv-array.h, builtin/revert.c, exec_cmd.h, git-compat-util.h, run-command.h

diff --git a/argv-array.h b/argv-array.h
index e805748..9a4c053 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -15,7 +15,7 @@ void argv_array_init(struct argv_array *);
 void argv_array_push(struct argv_array *, const char *);
 __attribute__((format (printf,2,3)))
 void argv_array_pushf(struct argv_array *, const char *fmt, ...);
-__attribute__((sentinel))
+GIT_SENTINEL(0)
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pop(struct argv_array *);
 void argv_array_clear(struct argv_array *);
diff --git a/builtin/revert.c b/builtin/revert.c
index b8b5174..219f354 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -54,7 +54,7 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
-__attribute__((sentinel))
+GIT_SENTINEL(0)
 static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 {
 	const char *this_opt;
@@ -71,7 +71,7 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 		die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
 }
 
-__attribute__((sentinel))
+GIT_SENTINEL(0)
 static void verify_opt_mutually_compatible(const char *me, ...)
 {
 	const char *opt1, *opt2 = NULL;
diff --git a/exec_cmd.h b/exec_cmd.h
index 307b55c..fbbb44d 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -7,7 +7,7 @@ extern const char *git_exec_path(void);
 extern void setup_path(void);
 extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
-__attribute__((sentinel))
+GIT_SENTINEL(0)
 extern int execl_git_cmd(const char *cmd, ...);
 extern const char *system_path(const char *path);
 
diff --git a/git-compat-util.h b/git-compat-util.h
index ff193f4..cac8157 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -303,6 +303,13 @@ extern char *gitbasename(char *);
 #endif
 #endif
 
+/* The sentinel attribute is valid from gcc version 4.0 */
+#if defined(__GNUC__) && (__GNUC__ >= 4)
+#define GIT_SENTINEL(n) __attribute__((sentinel(n)))
+#else
+#define GIT_SENTINEL(n)
+#endif
+
 #include "compat/bswap.h"
 
 #ifdef USE_WILDMATCH
diff --git a/run-command.h b/run-command.h
index 0a47679..2f2b453 100644
--- a/run-command.h
+++ b/run-command.h
@@ -46,7 +46,7 @@ int finish_command(struct child_process *);
 int run_command(struct child_process *);
 
 extern char *find_hook(const char *name);
-__attribute__((sentinel))
+GIT_SENTINEL(0)
 extern int run_hook(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1

-- 
1.8.3
Jeff King· Jul 18, 2013, 20:31 UTC · re: Ramsay Jones · lore

Re: [PATCH] Add the GIT_SENTINEL macro

On Thu, Jul 18, 2013 at 09:02:12PM +0100, Ramsay Jones wrote:
Show 9 quoted lines
> The sentinel function attribute is not understood by versions of
> the gcc compiler prior to v4.0. At present, for earlier versions
> of gcc, the build issues 108 warnings related to the unknown
> attribute. In order to suppress the warnings, we conditionally
> define the GIT_SENTINEL macro to provide the sentinel attribute
> for gcc v4.0 and newer.
> 
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
Acked-by: Jeff King <peff@peff.net>
> -__attribute__((sentinel))
> +GIT_SENTINEL(0)
>  void argv_array_pushl(struct argv_array *, ...);

We could also add GIT_SENTINEL to handle the default-zero case, but I do not think it is worth the trouble.

-Peff
Jonathan Nieder· Jul 18, 2013, 21:06 UTC · re: Ramsay Jones · lore

Re: [PATCH] Add the GIT_SENTINEL macro

Ramsay Jones wrote:
> This was built on the next branch

All the uses of the sentinel attribute seem to come from eccb6149 (use "sentinel" function attribute for variadic lists, 2013-07-09), so this should be okay to go on top of the jk/gcc-function-attributes branch.

Show 12 quoted lines
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -303,6 +303,13 @@ extern char *gitbasename(char *);
>  #endif
>  #endif
>  
> +/* The sentinel attribute is valid from gcc version 4.0 */
> +#if defined(__GNUC__) && (__GNUC__ >= 4)
> +#define GIT_SENTINEL(n) __attribute__((sentinel(n)))
> +#else
> +#define GIT_SENTINEL(n)
> +#endif
I'd mildly prefer
	#if ...
	#define GIT_SENTINEL __attribute__((sentinel))
	#else
	...

(without the numeric parameter). I don't know any function in git (or any other project for that matter) that takes extra parameters after the NULL sentinel.

But I don't care much, so
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks for a pleasant patch. Jonathan

Junio C Hamano· Jul 19, 2013, 03:30 UTC · re: Ramsay Jones · lore

Re: [PATCH] Add the GIT_SENTINEL macro

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
Show 11 quoted lines
> The sentinel function attribute is not understood by versions of
> the gcc compiler prior to v4.0. At present, for earlier versions
> of gcc, the build issues 108 warnings related to the unknown
> attribute. In order to suppress the warnings, we conditionally
> define the GIT_SENTINEL macro to provide the sentinel attribute
> for gcc v4.0 and newer.
>
> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
> ---
>
> This was built on the next branch
It seems to apply well on the tip of jk/gcc-function-attributes.
 - This macro is not about "git" at all, so I'll edit the patch to
   call it GCC_ATTR_SENTINEL before applying.
 - Also I'll drop the (0) at the end.
Thanks.
Jonathan Nieder· Jul 19, 2013, 03:36 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add the GIT_SENTINEL macro

Junio C Hamano wrote:
> It seems to apply well on the tip of jk/gcc-function-attributes.
>
>  - This macro is not about "git" at all, so I'll edit the patch to
>    call it GCC_ATTR_SENTINEL before applying.

Would naming it something like LAST_ARG_MUST_BE_NULL instead make sense? That way, if some other compiler gains a different syntax for the same annotation, it would be possible to do

	#if defined(__GNUC__) && __GNUC__ >= 4
	# define LAST_ARG_MUST_BE_NULL __attribute__((sentinel))
	#elif defined(_MSC_VER) && _MSC_VER > 27
	# define LAST_ARG_MUST_BE_NULL __declspec(lastargnull)
	#else
	# define LAST_ARG_MUST_BE_NULL
	#endif
Junio C Hamano· Jul 19, 2013, 07:18 UTC · re: Jonathan Nieder · lore

Re: [PATCH] Add the GIT_SENTINEL macro

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 18 quoted lines
> Junio C Hamano wrote:
>
>> It seems to apply well on the tip of jk/gcc-function-attributes.
>>
>>  - This macro is not about "git" at all, so I'll edit the patch to
>>    call it GCC_ATTR_SENTINEL before applying.
>
> Would naming it something like LAST_ARG_MUST_BE_NULL instead make
> sense?  That way, if some other compiler gains a different syntax for
> the same annotation, it would be possible to do
>
> 	#if defined(__GNUC__) && __GNUC__ >= 4
> 	# define LAST_ARG_MUST_BE_NULL __attribute__((sentinel))
> 	#elif defined(_MSC_VER) && _MSC_VER > 27
> 	# define LAST_ARG_MUST_BE_NULL __declspec(lastargnull)
> 	#else
> 	# define LAST_ARG_MUST_BE_NULL
> 	#endif

I do like last-arg-must-be-null name for its descriptiveness; with the example of NORETURN vs NORETURN_PTR, however, I am not quite convinced that it would help reusing the same macro to different compilers.

I would say it is already sheer luck that GCC's __attribute__(()) and MSC's __declspec() both come at the same place in the function declaration syntax, i.e. before the usual declaration. Your next compiler may want the magic immediately before the closing semicolon of the declaration, for example.

Ramsay Jones· Jul 20, 2013, 19:13 UTC · re: Junio C Hamano · lore

Re: [PATCH] Add the GIT_SENTINEL macro

Junio C Hamano wrote:
Show 23 quoted lines
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> The sentinel function attribute is not understood by versions of
>> the gcc compiler prior to v4.0. At present, for earlier versions
>> of gcc, the build issues 108 warnings related to the unknown
>> attribute. In order to suppress the warnings, we conditionally
>> define the GIT_SENTINEL macro to provide the sentinel attribute
>> for gcc v4.0 and newer.
>>
>> Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
>> ---
>>
>> This was built on the next branch
> 
> It seems to apply well on the tip of jk/gcc-function-attributes.
> 
> 
>  - This macro is not about "git" at all, so I'll edit the patch to
>    call it GCC_ATTR_SENTINEL before applying.
> 
>  - Also I'll drop the (0) at the end.
> 
> Thanks.

Yes, GCC_ATTR_SENTINEL is a better name, but I like LAST_ARG_MUST_BE_NULL even more! The final commit 9fe3edc4 ("Add the LAST_ARG_MUST_BE_NULL macro", 18-07-2013) is much better than my patch and works beautifully.

Thanks Junio, Jeff and Jonathan!

ATB, Ramsay Jones

← back to recent threads