[RFC/PATCH] Add the NO_SENTINEL build variable

[RFC/PATCH] Add the NO_SENTINEL build variable

[Ramsay Jones]

Signed-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>
---

Hi Jeff,

One of the three gcc compilers that I use does not understand the
sentinel function attribute. (so, it spews 108 warning messages)

Is this, or something like it, too ugly for you to squash into
your patch? :-D

ATB,
Ramsay Jones


 Makefile          | 6 ++++++
 argv-array.h      | 2 +-
 builtin/revert.c  | 4 ++--
 exec_cmd.h        | 2 +-
 git-compat-util.h | 6 ++++++
 run-command.h     | 2 +-
 6 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 9c0da06..63b539c 100644
--- a/Makefile
+++ b/Makefile
@@ -224,6 +224,9 @@ all::
 # Define NO_NORETURN if using buggy versions of gcc 4.6+ and profile feedback,
 # as the compiler can crash (http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49299)
 #
+# Define NO_SENTINEL if you have a compiler which does not understand the
+# sentinel function attribute.
+#
 # Define USE_NSEC below if you want git to care about sub-second file mtimes
 # and ctimes. Note that you need recent glibc (at least 2.2.4) for this, and
 # it will BREAK YOUR LOCAL DIFFS! show-diff and anything using it will likely
@@ -1232,6 +1235,9 @@ endif
 ifdef NO_NORETURN
 	BASIC_CFLAGS += -DNO_NORETURN
 endif
+ifdef NO_SENTINEL
+	BASIC_CFLAGS += -DNO_SENTINEL
+endif
 ifdef NO_NSEC
 	BASIC_CFLAGS += -DNO_NSEC
 endif
diff --git a/argv-array.h b/argv-array.h
index e805748..31bc492 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))
+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..6aedc18 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))
+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))
+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..75c0a82 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))
+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 9f1eaca..e846e01 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -300,6 +300,12 @@ extern char *gitbasename(char *);
 #endif
 #endif
 
+#if defined(__GNUC__) && !defined(NO_SENTINEL)
+#define SENTINEL(n) __attribute__((sentinel(n)))
+#else
+#define SENTINEL(n)
+#endif
+
 #include "compat/bswap.h"
 
 #ifdef USE_WILDMATCH
diff --git a/run-command.h b/run-command.h
index 0a47679..8e75671 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))
+SENTINEL(0)
 extern int run_hook(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
-- 
1.8.3

Re: [RFC/PATCH] Add the NO_SENTINEL build variable

[Jonathan Nieder]

Ramsay Jones wrote:

> One of the three gcc compilers that I use does not understand the
> sentinel function attribute. (so, it spews 108 warning messages)

Do you know what version of gcc introduced the sentinel attribute?
Would it make sense for the ifdef in git-compat-util.h to be keyed on 
__GNUC__ and __GNUC_MINOR__ instead of a new makefile flag?

Thanks,
Jonathan

Re: [RFC/PATCH] Add the NO_SENTINEL build variable

[Ramsay Jones]

Jonathan Nieder wrote:
> Ramsay Jones wrote:
> 
>> One of the three gcc compilers that I use does not understand the
>> sentinel function attribute. (so, it spews 108 warning messages)
> 
> Do you know what version of gcc introduced the sentinel attribute?
> Would it make sense for the ifdef in git-compat-util.h to be keyed on 
> __GNUC__ and __GNUC_MINOR__ instead of a new makefile flag?
> 

I have on old (v4.2.1) gcc repo on Linux and looking at

    ~/gcc-4.2.1/gcc/ChangeLog-2004

I can see that the sentinel attribute was added on 2004-09-04 by
Kaveh R. Ghazi.

Also, I find "bump version string to version 4.0.0" was on 2004-09-09
and "bump version string to version 3.5.0" was on 2004-01-16.

Several of my system header files (on Linux) imply that the
sentinel attribute is supported by __GNUC__ >= 4. (One of them,
ansidecl.h, states that gcc 3.5 supports it but ...)

ATB,
Ramsay Jones

Re: [RFC/PATCH] Add the NO_SENTINEL build variable

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> Jonathan Nieder wrote:
>> Ramsay Jones wrote:
>> 
>>> One of the three gcc compilers that I use does not understand the
>>> sentinel function attribute. (so, it spews 108 warning messages)
>> 
>> Do you know what version of gcc introduced the sentinel attribute?
>> Would it make sense for the ifdef in git-compat-util.h to be keyed on 
>> __GNUC__ and __GNUC_MINOR__ instead of a new makefile flag?
>> 
>
> I have on old (v4.2.1) gcc repo on Linux and looking at
>
>     ~/gcc-4.2.1/gcc/ChangeLog-2004
>
> I can see that the sentinel attribute was added on 2004-09-04 by
> Kaveh R. Ghazi.
>
> Also, I find "bump version string to version 4.0.0" was on 2004-09-09
> and "bump version string to version 3.5.0" was on 2004-01-16.
>
> Several of my system header files (on Linux) imply that the
> sentinel attribute is supported by __GNUC__ >= 4. (One of them,
> ansidecl.h, states that gcc 3.5 supports it but ...)

Perhaps a message from yesterday would have helped?

 http://article.gmane.org/gmane.comp.version-control.git/230633

seems to indicate that checking for version 4 is sufficient.

Also I asked you to split the __attribute__((sentinel(n)) support
into a separate patch.  We currently do not pass anything but 0
(meaning, the sentinel is always at the end), and SENTINEL in all
capital is easy enough to grep for when somebody _does_ want to have
such a support, so I'd prefer not to see __attribute__((sentinel(n))
until it becomes necessary.

Thanks.

Re: [RFC/PATCH] Add the NO_SENTINEL build variable

[Ramsay Jones]

Junio C Hamano wrote:
> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
> 
>> Jonathan Nieder wrote:
>>> Ramsay Jones wrote:
>>>
>>>> One of the three gcc compilers that I use does not understand the
>>>> sentinel function attribute. (so, it spews 108 warning messages)
>>>
>>> Do you know what version of gcc introduced the sentinel attribute?
>>> Would it make sense for the ifdef in git-compat-util.h to be keyed on 
>>> __GNUC__ and __GNUC_MINOR__ instead of a new makefile flag?
>>>
>>
>> I have on old (v4.2.1) gcc repo on Linux and looking at
>>
>>     ~/gcc-4.2.1/gcc/ChangeLog-2004
>>
>> I can see that the sentinel attribute was added on 2004-09-04 by
>> Kaveh R. Ghazi.
>>
>> Also, I find "bump version string to version 4.0.0" was on 2004-09-09
>> and "bump version string to version 3.5.0" was on 2004-01-16.
>>
>> Several of my system header files (on Linux) imply that the
>> sentinel attribute is supported by __GNUC__ >= 4. (One of them,
>> ansidecl.h, states that gcc 3.5 supports it but ...)
> 
> Perhaps a message from yesterday would have helped?
> 
>  http://article.gmane.org/gmane.comp.version-control.git/230633
> 
> seems to indicate that checking for version 4 is sufficient.
> 
> Also I asked you to split the __attribute__((sentinel(n)) support
> into a separate patch.  We currently do not pass anything but 0
> (meaning, the sentinel is always at the end), and SENTINEL in all
> capital is easy enough to grep for when somebody _does_ want to have
> such a support, so I'd prefer not to see __attribute__((sentinel(n))
> until it becomes necessary.

Sorry, but I didn't see any of these emails before sending this
and the subsequent patch email. :(

I can sometimes be away from email for several days at a time (and
then spend days trying to read the backlog - I'm on *far* too many
mailing lists!).

[The internet went black on me last night; I couldn't see anything
outside of my ISP's servers (and not all of those). I also couldn't
get any answer at the support phone number, so I probably wasn't the
only one ...]

ATB,
Ramsay Jones

Re: [RFC/PATCH] Add the NO_SENTINEL build variable

[Junio C Hamano]

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 9f1eaca..e846e01 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -300,6 +300,12 @@ extern char *gitbasename(char *);
>  #endif
>  #endif
>  
> +#if defined(__GNUC__) && !defined(NO_SENTINEL)
> +#define SENTINEL(n) __attribute__((sentinel(n)))
> +#else
> +#define SENTINEL(n)
> +#endif

Allowing callers to use __attribute__((sentinel(1)), while it may be
a good enhancement, does not belong to "some versions of GCC do not
know what to do with the sentinel attribute".  Making this change in
a separate patch on top would be cleaner.

Also do we know what version of GCC started supporting this
attribute?  http://gcc.gnu.org/gcc-4.0/changes.html mentions it in
"New Languages and Language specific improvements" section, but the
page also says "The latest release in the 4.0 release series is GCC
4.0.4", so it is not clear if 4.0 had it, or it was added somewhere
between 4.0 and 4.0.4 to me.

Re: [RFC/PATCH] Add the NO_SENTINEL build variable

[Andreas Schwab]

Junio C Hamano <gitster@pobox.com> writes:

> Also do we know what version of GCC started supporting this
> attribute?  http://gcc.gnu.org/gcc-4.0/changes.html mentions it in
> "New Languages and Language specific improvements" section, but the
> page also says "The latest release in the 4.0 release series is GCC
> 4.0.4", so it is not clear if 4.0 had it, or it was added somewhere
> between 4.0 and 4.0.4 to me.

Generally, gcc doesn't get new features added within the same minor
version, only bug fixes.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."