{"thread":{"id":"39193","subject":"[PATCH v3] git-compat-util.h: implement a different ARRAY_SIZE macro for for safely deriving the size of array","startedAt":"2015-04-30T12:44:14Z","lastAt":"2015-04-30T18:19:01Z","messageCount":7,"participants":["Elia Pinto","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"260272","messageId":"1430397854-28908-1-git-send-email-gitter.spiros@gmail.com","threadId":"39193","inReplyTo":null,"subject":"[PATCH v3] git-compat-util.h: implement a different ARRAY_SIZE macro for for safely deriving the size of array","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2015-04-30T12:44:14Z","receivedAt":"2015-04-30T12:44:14Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"To get number of elements in an array git use the ARRAY_SIZE macro\ndefined as:\n\n #define ARRAY_SIZE(x) (sizeof(x)/sizeof((x)[0]))\n\nThe problem with it is a possibility of mistakenly passing to it a\npointer instead an array. The ARRAY_SIZE macro as conventionally\ndefined does not provide good type-safety and the open-coded\napproach is more fragile, more verbose and provides no improvement in\ntype-safety.\n\nUse instead a different but compatible ARRAY_SIZE() macro,\nwhich will also break compile if you try to\nuse it on a pointer. This implemention revert to the original code\nif the compiler doesn't know the typeof and __builtin_types_compatible_p\nGCC extensions.\n\nThis can ensure our code is robust to changes, without\nneeding a gratuitous macro or constant. A similar\nARRAY_SIZE implementation also exists in the linux kernel.\n\nCredits to Rusty Russell and his ccan library.\n\nSigned-off-by: Elia Pinto \n---\n git-compat-util.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 53 insertions(+), 1 deletion(-)\n\nThis is the third version of the patch.\n\nCompared to the second version:\n\n- Eliminated the autoconf use. I use instead the GCC (and compatible compilers) macros\n for checking if the not portable builtin is supported or not (\"Jeff suggestion\")\n- Changed the name of the macro from _array_size_chk to BARF_IF_IS_NOT_AN_ARRAY i\n (\"Junio suggestion. In ALL_CAPS for the Jeff comment )\"\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex bc8fc8c..b29c9f3 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -3,6 +3,23 @@\n \n #define _FILE_OFFSET_BITS 64\n \n+\n+/* Derived from Linux \"Features Test Macro\" header\n+ * Convenience macros to test the versions of gcc (or \n+ * a compatible compiler).\n+ * Use them like this:\n+ * #if GIT_GNUC_PREREQ (2,8)\n+ * ... code requiring gcc 2.8 or later ...\n+ * #endif\n+*/\n+#if defined(__GNUC__) && defined(__GNUC_MINOR__)\n+# define GIT_GNUC_PREREQ(maj, min) \\\n+\t((__GNUC__ << 16) + __GNUC_MINOR__ >= ((maj) << 16) + (min))\n+#else\n+ #define GIT_GNUC_PREREQ(maj, min) 0\n+#endif\n+\n+\n #ifndef FLEX_ARRAY\n /*\n * See if our compiler is known to support flexible array members.\n@@ -25,7 +42,42 @@\n #endif\n #endif\n \n-#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))\n+\n+/*\n+ * BUILD_ASSERT_OR_ZERO - assert a build-time dependency, as an expression.\n+ * @cond: the compile-time condition which must be true.\n+ *\n+ * Your compile will fail if the condition isn't true, or can't be evaluated\n+ * by the compiler. This can be used in an expression: its value is \"0\".\n+ *\n+ * Example:\n+ *\t#define foo_to_char(foo)\t\t\t\t\t\\\n+ *\t\t ((char *)(foo)\t\t\t\t\t\t\\\n+ *\t\t + BUILD_ASSERT_OR_ZERO(offsetof(struct foo, string) == 0))\n+ */\n+#define BUILD_ASSERT_OR_ZERO(cond) \\\n+\t(sizeof(char [1 - 2*!(cond)]) - 1)\n+\n+#if defined(__GNUC__) && (__GNUC__ >= 3)\n+# if GIT_GNUC_PREREQ(3, 1)\n+ /* &arr[0] degrades to a pointer: a different type from an array */\n+# define BARF_IF_IS_NOT_AN_ARRAY(arr)\t\t\t\t\t\t\\\n+\tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(typeof(arr), \\\n+\t\t\t\t\t\t\ttypeof(&(arr)[0])))\n+# else\n+# define BARF_IF_IS_NOT_AN_ARRAY(arr) 0 \n+# endif\n+#endif \n+/*\n+ * ARRAY_SIZE - get the number of elements in a visible array\n+ * x: the array whose size you want.\n+ *\n+ * This does not work on pointers, or arrays declared as [], or\n+ * function parameters. With correct compiler support, such usage\n+ * will cause a build error (see the build_assert_or_zero macro).\n+ */\n+#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + BARF_IF_IS_NOT_AN_ARRAY(x))\n+\n #define bitsizeof(x) (CHAR_BIT * sizeof(x))\n \n #define maximum_signed_value_of_type(a) \\\n-- \n2.1.0\n"},{"id":"260277","messageId":"xmqqk2wt1qk4.fsf@gitster.dls.corp.google.com","threadId":"39193","inReplyTo":"1430397854-28908-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH v3] git-compat-util.h: implement a different ARRAY_SIZE macro for for safely deriving the size of array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-30T17:20:11Z","receivedAt":"2015-04-30T17:20:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> To get number of elements in an array git use the ARRAY_SIZE macro\n> defined as:\n>\n> #define ARRAY_SIZE(x) (sizeof(x)/sizeof((x)[0]))\n>\n> The problem with it is a possibility of mistakenly passing to it a\n> pointer instead an array. The ARRAY_SIZE macro as conventionally\n> defined does not provide good type-safety and the open-coded\n> approach is more fragile, more verbose and provides no improvement in\n> type-safety.\n>\n> Use instead a different but compatible ARRAY_SIZE() macro,\n> which will also break compile if you try to\n> use it on a pointer. This implemention revert to the original code\n> if the compiler doesn't know the typeof and __builtin_types_compatible_p\n> GCC extensions.\n>\n> This can ensure our code is robust to changes, without\n> needing a gratuitous macro or constant. A similar\n> ARRAY_SIZE implementation also exists in the linux kernel.\n>\n> Credits to Rusty Russell and his ccan library.\n>\n> Signed-off-by: Elia Pinto \n> ---\n> git-compat-util.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 53 insertions(+), 1 deletion(-)\n>\n> This is the third version of the patch.\n>\n> Compared to the second version:\n>\n> - Eliminated the autoconf use. I use instead the GCC (and compatible compilers) macros\n> for checking if the not portable builtin is supported or not (\"Jeff suggestion\")\n> - Changed the name of the macro from _array_size_chk to BARF_IF_IS_NOT_AN_ARRAY i\n> (\"Junio suggestion. In ALL_CAPS for the Jeff comment )\"\n\nThanks. Allow me to s/BARF_IF_IS_NOT_AN_ARRAY/BARF_UNLESS_AN_ARRAY/\neverywhere for brevity while applying.\n"},{"id":"260278","messageId":"xmqqd22l1pkl.fsf@gitster.dls.corp.google.com","threadId":"39193","inReplyTo":"1430397854-28908-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH v3] git-compat-util.h: implement a different ARRAY_SIZE macro for for safely deriving the size of array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-30T17:41:30Z","receivedAt":"2015-04-30T17:41:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> +#if defined(__GNUC__) && (__GNUC__ >= 3)\n> +# if GIT_GNUC_PREREQ(3, 1)\n> + /* &arr[0] degrades to a pointer: a different type from an array */\n> +# define BARF_IF_IS_NOT_AN_ARRAY(arr)\t\t\t\t\t\t\\\n> +\tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(typeof(arr), \\\n> +\t\t\t\t\t\t\ttypeof(&(arr)[0])))\n> +# else\n> +# define BARF_IF_IS_NOT_AN_ARRAY(arr) 0 \n> +# endif\n> +#endif \n> +/*\n> + * ARRAY_SIZE - get the number of elements in a visible array\n> + * x: the array whose size you want.\n> + *\n> + * This does not work on pointers, or arrays declared as [], or\n> + * function parameters. With correct compiler support, such usage\n> + * will cause a build error (see the build_assert_or_zero macro).\n> + */\n> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]) + BARF_IF_IS_NOT_AN_ARRAY(x))\n> +\n> #define bitsizeof(x) (CHAR_BIT * sizeof(x))\n> \n> #define maximum_signed_value_of_type(a) \\\n\nHmmmmmmmm.\n\n CC ws.o\nws.c: In function 'parse_whitespace_rule':\nws.c:46:3: error: unknown type name 'typeof'\n for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++) {\n ^\n\n$ gcc --version\ngcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2\nCopyright (C) 2013 Free Software Foundation, Inc.\nThis is free software; see the source for copying conditions. There\nis NO\nwarranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR\nPURPOSE.\n"},{"id":"260279","messageId":"20150430174440.GC21800@peff.net","threadId":"39193","inReplyTo":"xmqqd22l1pkl.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] git-compat-util.h: implement a different ARRAY_SIZE macro for for safely deriving the size of array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-30T17:44:41Z","receivedAt":"2015-04-30T17:44:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 30, 2015 at 10:41:30AM -0700, Junio C Hamano wrote:\n\n> Elia Pinto writes:\n> \n> > +#if defined(__GNUC__) && (__GNUC__ >= 3)\n> > +# if GIT_GNUC_PREREQ(3, 1)\n> > + /* &arr[0] degrades to a pointer: a different type from an array */\n> > +# define BARF_IF_IS_NOT_AN_ARRAY(arr)\t\t\t\t\t\t\\\n> > +\tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(typeof(arr), \\\n> > +\t\t\t\t\t\t\ttypeof(&(arr)[0])))\n> > +# else\n> > +# define BARF_IF_IS_NOT_AN_ARRAY(arr) 0 \n> > +# endif\n> [...]\n> \n> Hmmmmmmmm.\n> \n> CC ws.o\n> ws.c: In function 'parse_whitespace_rule':\n> ws.c:46:3: error: unknown type name 'typeof'\n> for (i = 0; i < ARRAY_SIZE(whitespace_rule_names); i++) {\n> ^\n> \n> $ gcc --version\n> gcc (Ubuntu 4.8.2-19ubuntu1) 4.8.2\n> Copyright (C) 2013 Free Software Foundation, Inc.\n> This is free software; see the source for copying conditions. There\n> is NO\n> warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR\n> PURPOSE.\n\nISTR that you compile with \"-std=c89\". typeof was added in c99, I think\n(and was a GNU extension before that). I wonder if that is fooling the\ngcc version-check.\n\n-Peff\n"},{"id":"260280","messageId":"xmqq8ud91ozm.fsf@gitster.dls.corp.google.com","threadId":"39193","inReplyTo":"20150430174440.GC21800@peff.net","subject":"Re: [PATCH v3] git-compat-util.h: implement a different ARRAY_SIZE macro for for safely deriving the size of array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-30T17:54:05Z","receivedAt":"2015-04-30T17:54:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ISTR that you compile with \"-std=c89\". typeof was added in c99, I think\n> (and was a GNU extension before that). I wonder if that is fooling the\n> gcc version-check.\n\nYeah, I think that is very likely.\n"},{"id":"260281","messageId":"xmqq4mnx1or9.fsf@gitster.dls.corp.google.com","threadId":"39193","inReplyTo":"xmqq8ud91ozm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] git-compat-util.h: implement a different ARRAY_SIZE macro for for safely deriving the size of array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-30T17:59:06Z","receivedAt":"2015-04-30T17:59:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> ISTR that you compile with \"-std=c89\". typeof was added in c99, I think\n>> (and was a GNU extension before that). I wonder if that is fooling the\n>> gcc version-check.\n>\n> Yeah, I think that is very likely.\n\nYes. One workaround is to explicitly say that we accept the GNU\nextension in the source, perhaps.\n\n git-compat-util.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f1f8f23..7fad5aa 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -62,8 +62,8 @@\n # if GIT_GNUC_PREREQ(3, 1)\n /* &arr[0] degrades to a pointer: a different type from an array */\n # define BARF_UNLESS_AN_ARRAY(arr)\t\t\t\t\t\t\\\n-\tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(typeof(arr), \\\n-\t\t\t\t\t\t\ttypeof(&(arr)[0])))\n+\tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \\\n+\t\t\t\t\t\t\t __typeof__(&(arr)[0])))\n # else\n # define BARF_UNLESS_AN_ARRAY(arr) 0\n # endif\n"},{"id":"260283","messageId":"20150430181901.GA23431@peff.net","threadId":"39193","inReplyTo":"xmqq4mnx1or9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] git-compat-util.h: implement a different ARRAY_SIZE macro for for safely deriving the size of array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-30T18:19:01Z","receivedAt":"2015-04-30T18:19:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 30, 2015 at 10:59:06AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano writes:\n> \n> > Jeff King writes:\n> >\n> >> ISTR that you compile with \"-std=c89\". typeof was added in c99, I think\n> >> (and was a GNU extension before that). I wonder if that is fooling the\n> >> gcc version-check.\n> >\n> > Yeah, I think that is very likely.\n> \n> Yes. One workaround is to explicitly say that we accept the GNU\n> extension in the source, perhaps.\n> \n> git-compat-util.h | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index f1f8f23..7fad5aa 100644\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -62,8 +62,8 @@\n> # if GIT_GNUC_PREREQ(3, 1)\n> /* &arr[0] degrades to a pointer: a different type from an array */\n> # define BARF_UNLESS_AN_ARRAY(arr)\t\t\t\t\t\t\\\n> -\tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(typeof(arr), \\\n> -\t\t\t\t\t\t\ttypeof(&(arr)[0])))\n> +\tBUILD_ASSERT_OR_ZERO(!__builtin_types_compatible_p(__typeof__(arr), \\\n> +\t\t\t\t\t\t\t __typeof__(&(arr)[0])))\n\nThat seems like a reasonable fix, provided that other gcc-compatible\ncompilers implement the GNU-compatible name. I just tried with clang\n3.5, and it seems to work fine.\n\nAnd I was wrong about \"typeof\". It is not in C99 at all. So I think it\nis really a matter of whether you are in the default \"-std=gnu89\" mode\nor not, as to whether the compiler imports the \"typeof\" keyword, or if\nit must be hidden in the compiler-reserved area (starting with \"__\").\n\n-Peff\n"}]}