[PATCH] Add the GIT_SENTINEL macro

[PATCH] Add the GIT_SENTINEL macro

[Ramsay Jones]

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(-)

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

Re: [PATCH] Add the GIT_SENTINEL macro

[Jeff King]

On Thu, Jul 18, 2013 at 09:02:12PM +0100, Ramsay Jones wrote:

> 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

Re: [PATCH] Add the GIT_SENTINEL macro

[Jonathan Nieder]

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.

> --- 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

Re: [PATCH] Add the GIT_SENTINEL macro

[Junio C Hamano]

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.

Re: [PATCH] Add the GIT_SENTINEL macro

[Jonathan Nieder]

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

Re: [PATCH] Add the GIT_SENTINEL macro

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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.

Re: [PATCH] Add the GIT_SENTINEL macro

[Ramsay Jones]

Junio C Hamano wrote:
> 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