threads / rfc / 34444

RFC patchAdd the NO_SENTINEL build variable

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

## tl;dr

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

replies: 6people: 4as markdown or json

Ramsay Jones· Jul 15, 2013, 17:38 UTC · lore
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(-)
Show changes to 6 files +17 −5

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

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
Jonathan Nieder· Jul 15, 2013, 18:13 UTC · re: Ramsay Jones · lore

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

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

Ramsay Jones· Jul 18, 2013, 17:27 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
Show 9 quoted lines
> 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

Junio C Hamano· Jul 18, 2013, 21:25 UTC · re: Ramsay Jones · lore

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

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
Show 24 quoted lines
> 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.
Ramsay Jones· Jul 20, 2013, 17:51 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 39 quoted lines
> 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

Junio C Hamano· Jul 17, 2013, 16:06 UTC · re: Ramsay Jones · lore

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

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:
Show 13 quoted lines
> 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.

Andreas Schwab· Jul 17, 2013, 16:55 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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."

← back to recent threads