threads / rfc / 48839

RFC patch, 6 partsCompile cleanly in pedantic mode

Subject: [RFC PATCH 0/6] Compile cleanly in pedantic mode

## tl;dr

39 messages between Jul 8, 2018 and Jul 12, 2018. Diffs are folded; open one to read it.

replies: 38people: 7as markdown or json

Beat Bolli· Jul 8, 2018, 14:43 UTC · lore

While developing 6aaded550 ("builtin/config: work around an unsized array forward declaration", 2018-07-05), I have compiled Git with CFLAGS="-std=c99 -pedantic".

This is an RFC patch series that fixes a few compiler warnings when compiling with these options, always assuming that this is a worthwile goal.

Note that all warnings were produced by -pedantic; the C99 standard option by itself didn't cause any of them.

The warnings were:
1) Char arrays initialized from a parenthesized string.
        Suppressed by defining USE_PARENS_AROUND_GETTEXT_N to 0
        globally. This was done just to keep the amount of warnings
        manageable; this series leaves that knob alone. The advantage of
        not mistakenly concatenating two translated strings is greater.
2) connect.h, refs/refs-internal.h: Forward reference to an enum.
        Added two #includes that define the enums. This was already
        (inconclusively) talked about in [0].
3) convert.c: Invalid escape sequence "\e".
        Replaced with "\033".
4) seqencer.c: Empty statements at top level.
        Removed the extra semicolons.
5) string-list.c: Forbidden to cast from void * to a function pointer and
   vice versa.
        Encapsulated the function pointer in a context struct. This is
        controversial because it has a performance impact, namely one
        additional pointer dereference per string comparison. An
        alternative might be to use multiple casts via intptr_t. But
        I'm not sure if this is worth the trouble.
6) utf8.c: overflow of char values.
        Use unsigned char for the BOM constants.
This series has patches for 2) to 6).

Regards, Beat

[0] https://public-inbox.org/git/53ab8626-f862-a732-b369-abeab69a468f@ramsayjones.plus.com/T/
Beat Bolli· Jul 8, 2018, 14:43 UTC · re: Beat Bolli · lore

[RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

The marco GIT_PATH_FUNC expands to a complete statement including the semicolon. Remove two extra trailing semicolons.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to sequencer.c +2 −2
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..66e7073995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -62,12 +62,12 @@ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
  * The file to keep track of how many commands were already processed (e.g.
  * for the prompt).
  */
-static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
+static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum")
 /*
  * The file to keep track of how many commands are to be processed in total
  * (e.g. for the prompt).
  */
-static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
+static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end")
 /*
  * The commit message that is planned to be used for any changes that
  * need to be committed following a user interaction.
-- 
2.15.0.rc1.299.gda03b47c3
Eric Sunshine· Jul 8, 2018, 20:54 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

On Sun, Jul 8, 2018 at 10:44 AM Beat Bolli <dev+git@drbeat.li> wrote:
Show 7 quoted lines
> The marco GIT_PATH_FUNC expands to a complete statement including the
> semicolon. Remove two extra trailing semicolons.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  sequencer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

While you're at it, perhaps it would be a good idea to fix the example in path.h which teaches the "wrong" way:

/*
 * You can define a static memoized git path like:
 *
 *    static GIT_PATH_FUNC(git_path_foo, "FOO");
 *
 * or use one of the global ones below.
 */
Philip Oakley· Jul 8, 2018, 21:17 UTC · re: Eric Sunshine · lore

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

From: "Eric Sunshine" <sunshine@sunshineco.com>
To: "Beat Bolli" <dev+git@drbeat.li>
> On Sun, Jul 8, 2018 at 10:44 AM Beat Bolli <dev+git@drbeat.li> wrote:
>> The marco GIT_PATH_FUNC expands to a complete statement including the
s/marco/macro/
Show 18 quoted lines
>> semicolon. Remove two extra trailing semicolons.
>>
>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>> ---
>>  sequencer.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> While you're at it, perhaps it would be a good idea to fix the example
> in path.h which teaches the "wrong" way:
> 
> /*
> * You can define a static memoized git path like:
> *
> *    static GIT_PATH_FUNC(git_path_foo, "FOO");
> *
> * or use one of the global ones below.
> */
>
ig@drbeat.li· Jul 9, 2018, 09:37 UTC · re: Philip Oakley · lore

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

Am 08.07.2018 23:17, schrieb Philip Oakley:
Show 6 quoted lines
> From: "Eric Sunshine" <sunshine@sunshineco.com>
> To: "Beat Bolli" <dev+git@drbeat.li>
>> On Sun, Jul 8, 2018 at 10:44 AM Beat Bolli <dev+git@drbeat.li> wrote:
>>> The marco GIT_PATH_FUNC expands to a complete statement including the
> 
> s/marco/macro/

ACK. In addition, the whole sentence is wrong: GIT_PATH_FUNC defines a function, not a statement. Will be fixes in a reroll.

Show 17 quoted lines
>>> semicolon. Remove two extra trailing semicolons.
>>> 
>>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>>> ---
>>>  sequencer.c | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> While you're at it, perhaps it would be a good idea to fix the example
>> in path.h which teaches the "wrong" way:
>> 
>> /*
>> * You can define a static memoized git path like:
>> *
>> *    static GIT_PATH_FUNC(git_path_foo, "FOO");
>> *
>> * or use one of the global ones below.
>> */

Thanks, Beat

Junio C Hamano· Jul 9, 2018, 21:34 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

Beat Bolli <dev+git@drbeat.li> writes:
> The marco GIT_PATH_FUNC expands to a complete statement including the
> semicolon. Remove two extra trailing semicolons.

Wait a bit. The observation in the log message and the implementation of GIT_PATH_FUNC() do not match.

        #define GIT_PATH_FUNC(func, filename) \
                const char *func(void) \
                { \
                        static char *ret; \
                        if (!ret) \
                                ret = git_pathdup(filename); \
                        return ret; \
                }

The code generated does "include semicolon" but that is not why the caller should place semicolon after the closing parens. Perhaps replace "including the semicolon." with something else, like ", and adding a semicolon after it not only is unnecessary but is wrong." or soemthing like that?

It is a bit unfortunate that we need to live with a slight uglyness of the resulting source code, unlike e.g. define_commit_slab() that can (and must) end with a semicolon, which gives us a more natural look. But that is a separate issue.

Show 25 quoted lines
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  sequencer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..66e7073995 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -62,12 +62,12 @@ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
>   * The file to keep track of how many commands were already processed (e.g.
>   * for the prompt).
>   */
> -static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
> +static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum")
>  /*
>   * The file to keep track of how many commands are to be processed in total
>   * (e.g. for the prompt).
>   */
> -static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
> +static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end")
>  /*
>   * The commit message that is planned to be used for any changes that
>   * need to be committed following a user interaction.
Beat Bolli· Jul 9, 2018, 21:37 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level

On 09.07.18 23:34, Junio C Hamano wrote:
Show 22 quoted lines
> Beat Bolli <dev+git@drbeat.li> writes:
> 
>> The marco GIT_PATH_FUNC expands to a complete statement including the
>> semicolon. Remove two extra trailing semicolons.
> 
> Wait a bit.  The observation in the log message and the
> implementation of GIT_PATH_FUNC() do not match.
> 
>         #define GIT_PATH_FUNC(func, filename) \
>                 const char *func(void) \
>                 { \
>                         static char *ret; \
>                         if (!ret) \
>                                 ret = git_pathdup(filename); \
>                         return ret; \
>                 }
> 
> The code generated does "include semicolon" but that is not why the
> caller should place semicolon after the closing parens.  Perhaps
> replace "including the semicolon." with something else, like ", and
> adding a semicolon after it not only is unnecessary but is wrong."
> or soemthing like that?

This message is fixed in the non-RFC series that I sent at 19:25 UTC. I noticed the error after the message from Philip Oakley.

Beat
Beat Bolli· Jul 8, 2018, 14:43 UTC · re: Beat Bolli · lore

[RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

Include iterator.h to define enum iterator_selection.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 refs/refs-internal.h | 2 ++
 1 file changed, 2 insertions(+)
Show changes to refs/refs-internal.h +2 −0
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dd834314bd..a78b5cb803 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -1,6 +1,8 @@
 #ifndef REFS_REFS_INTERNAL_H
 #define REFS_REFS_INTERNAL_H
 
+#include "iterator.h"   /* for enum iterator_selection */
+
 /*
  * Data structures and functions for the internal use of the refs
  * module. Code outside of the refs module should use only the public
-- 
2.15.0.rc1.299.gda03b47c3
Jeff King· Jul 9, 2018, 18:46 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

On Sun, Jul 08, 2018 at 04:43:38PM +0200, Beat Bolli wrote:
Show 9 quoted lines
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index dd834314bd..a78b5cb803 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -1,6 +1,8 @@
>  #ifndef REFS_REFS_INTERNAL_H
>  #define REFS_REFS_INTERNAL_H
>  
> +#include "iterator.h"   /* for enum iterator_selection */

IMHO this kind of comment does more harm than good, because it is so prone to going stale (nobody is going to bother updating it when they add new dependencies on iterator.h). Anybody who is interested in the original reason can use "git blame" to dig up your commit message. And anybody who is thinking about deleting that line would need to dig into whether anything had been added in the meantime that also requires the include.

So at best it's redundant, and at worst it's slightly misleading. :)

Not worth a re-roll by itself, but it looked like you had a few other bits in the other patches to address.

Other than this minor quibble, the whole series looks good to me, modulo the existing review.

-Peff
Beat Bolli· Jul 9, 2018, 19:30 UTC · re: Jeff King · lore

Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

On 09.07.18 20:46, Jeff King wrote:
Show 30 quoted lines
> On Sun, Jul 08, 2018 at 04:43:38PM +0200, Beat Bolli wrote:
> 
>> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
>> index dd834314bd..a78b5cb803 100644
>> --- a/refs/refs-internal.h
>> +++ b/refs/refs-internal.h
>> @@ -1,6 +1,8 @@
>>  #ifndef REFS_REFS_INTERNAL_H
>>  #define REFS_REFS_INTERNAL_H
>>  
>> +#include "iterator.h"   /* for enum iterator_selection */
> 
> IMHO this kind of comment does more harm than good, because it is so
> prone to going stale (nobody is going to bother updating it when they
> add new dependencies on iterator.h). Anybody who is interested in the
> original reason can use "git blame" to dig up your commit message. And
> anybody who is thinking about deleting that line would need to dig into
> whether anything had been added in the meantime that also requires the
> include.
> 
> So at best it's redundant, and at worst it's slightly misleading. :)
> 
> Not worth a re-roll by itself, but it looked like you had a few other
> bits in the other patches to address.
> 
> Other than this minor quibble, the whole series looks good to me, modulo
> the existing review.
> 
> -Peff
> 
Ooosp, I've just sent the non-RFC reroll without this change.

Junio, would you squash this into [1/6] and [2/6], please (if you agree, of course :-)

Beat
Jeff King· Jul 10, 2018, 02:15 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

On Mon, Jul 09, 2018 at 09:30:12PM +0200, Beat Bolli wrote:
Show 7 quoted lines
> > Other than this minor quibble, the whole series looks good to me, modulo
> > the existing review.
> 
> Ooosp, I've just sent the non-RFC reroll without this change.
> 
> Junio, would you squash this into [1/6] and [2/6], please (if you agree,
> of course :-)

:) I'd be happy with that squash, but not the end of the world if it doesn't make it. Thanks for the whole series.

-Peff
Beat Bolli· Jul 8, 2018, 14:43 UTC · re: Beat Bolli · lore

[RFC PATCH 3/6] convert.c: replace "\e" escapes with "\033".

The "\e" escape is not defined in ISO C.
While on this line, add a missing space after the comma.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 convert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to convert.c +1 −1
diff --git a/convert.c b/convert.c
index 64d0d30e08..edebb946f5 100644
--- a/convert.c
+++ b/convert.c
@@ -334,7 +334,7 @@ static void trace_encoding(const char *context, const char *path,
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(
-			&trace,"| \e[2m%2i:\e[0m %2x \e[2m%c\e[0m%c",
+			&trace, "| \033[2m%2i:\033[0m %2x \033[2m%c\033[0m%c",
 			i,
 			(unsigned char) buf[i],
 			(buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),
-- 
2.15.0.rc1.299.gda03b47c3
Beat Bolli· Jul 8, 2018, 14:43 UTC · re: Beat Bolli · lore

[RFC PATCH 6/6] utf8.c: avoid char overflow

In ISO C, char constants must be in the range -128..127. Change the BOM constants to unsigned char to avoid overflow.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 utf8.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)
Show changes to utf8.c +5 −5
diff --git a/utf8.c b/utf8.c
index d55e20c641..833ce00617 100644
--- a/utf8.c
+++ b/utf8.c
@@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int insz,
 #endif
 
 static int has_bom_prefix(const char *data, size_t len,
-			  const char *bom, size_t bom_len)
+			  const unsigned char *bom, size_t bom_len)
 {
 	return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
 }
 
-static const char utf16_be_bom[] = {0xFE, 0xFF};
-static const char utf16_le_bom[] = {0xFF, 0xFE};
-static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
-static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
+static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
+static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
+static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
 
 int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)
 {
-- 
2.15.0.rc1.299.gda03b47c3
Johannes Schindelin· Jul 9, 2018, 13:14 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

Hi Beat,
On Sun, 8 Jul 2018, Beat Bolli wrote:
Show 30 quoted lines
> In ISO C, char constants must be in the range -128..127. Change the BOM
> constants to unsigned char to avoid overflow.
> 
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  utf8.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/utf8.c b/utf8.c
> index d55e20c641..833ce00617 100644
> --- a/utf8.c
> +++ b/utf8.c
> @@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int insz,
>  #endif
>  
>  static int has_bom_prefix(const char *data, size_t len,
> -			  const char *bom, size_t bom_len)
> +			  const unsigned char *bom, size_t bom_len)
>  {
>  	return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
>  }
>  
> -static const char utf16_be_bom[] = {0xFE, 0xFF};
> -static const char utf16_le_bom[] = {0xFF, 0xFE};
> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};

An alternative approach that might be easier to read (and avoids the confusion arising from our use of (signed) chars for strings pretty much everywhere):

#define FE ((char)0xfe) #define FF ((char)0xff)

...

Ciao, Dscho

Beat Bolli· Jul 9, 2018, 14:48 UTC · re: Johannes Schindelin · lore

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

Hi Dscho
Am 09.07.2018 15:14, schrieb Johannes Schindelin:
Show 47 quoted lines
> Hi Beat,
> 
> On Sun, 8 Jul 2018, Beat Bolli wrote:
> 
>> In ISO C, char constants must be in the range -128..127. Change the 
>> BOM
>> constants to unsigned char to avoid overflow.
>> 
>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>> ---
>>  utf8.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>> 
>> diff --git a/utf8.c b/utf8.c
>> index d55e20c641..833ce00617 100644
>> --- a/utf8.c
>> +++ b/utf8.c
>> @@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int 
>> insz,
>>  #endif
>> 
>>  static int has_bom_prefix(const char *data, size_t len,
>> -			  const char *bom, size_t bom_len)
>> +			  const unsigned char *bom, size_t bom_len)
>>  {
>>  	return data && bom && (len >= bom_len) && !memcmp(data, bom, 
>> bom_len);
>>  }
>> 
>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
> 
> An alternative approach that might be easier to read (and avoids the
> confusion arising from our use of (signed) chars for strings pretty 
> much
> everywhere):
> 
> #define FE ((char)0xfe)
> #define FF ((char)0xff)
> 
> ...

I have tried this first (without the macros, though), and thought it looked really ugly. That's why I chose this solution. The usage is pretty local and close to function has_bom_prefix().

Would an explaining comment help?
Beat
Beat Bolli· Jul 9, 2018, 15:45 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

Am 09.07.2018 16:48, schrieb Beat Bolli:
Show 60 quoted lines
> Hi Dscho
> 
> Am 09.07.2018 15:14, schrieb Johannes Schindelin:
>> Hi Beat,
>> 
>> On Sun, 8 Jul 2018, Beat Bolli wrote:
>> 
>>> In ISO C, char constants must be in the range -128..127. Change the 
>>> BOM
>>> constants to unsigned char to avoid overflow.
>>> 
>>> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
>>> ---
>>>  utf8.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/utf8.c b/utf8.c
>>> index d55e20c641..833ce00617 100644
>>> --- a/utf8.c
>>> +++ b/utf8.c
>>> @@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int 
>>> insz,
>>>  #endif
>>> 
>>>  static int has_bom_prefix(const char *data, size_t len,
>>> -			  const char *bom, size_t bom_len)
>>> +			  const unsigned char *bom, size_t bom_len)
>>>  {
>>>  	return data && bom && (len >= bom_len) && !memcmp(data, bom, 
>>> bom_len);
>>>  }
>>> 
>>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 
>>> 0xFF};
>>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 
>>> 0x00};
>> 
>> An alternative approach that might be easier to read (and avoids the
>> confusion arising from our use of (signed) chars for strings pretty 
>> much
>> everywhere):
>> 
>> #define FE ((char)0xfe)
>> #define FF ((char)0xff)
>> 
>> ...
> 
> I have tried this first (without the macros, though), and thought it 
> looked
> really ugly. That's why I chose this solution. The usage is pretty 
> local and
> close to function has_bom_prefix().
> 
> Would an explaining comment help?
I have found an even simpler solution. Use proper char literals.
I will put this into v2.

Regards, Beat

Show changes to utf8.c +4 −4
diff --git a/utf8.c b/utf8.c
index d55e20c641..982217eec9 100644
--- a/utf8.c
+++ b/utf8.c
@@ -566,10 +566,10 @@ static int has_bom_prefix(const char *data, size_t 
len,
         return data && bom && (len >= bom_len) && !memcmp(data, bom, 
bom_len);
  }

-static const char utf16_be_bom[] = {0xFE, 0xFF};
-static const char utf16_le_bom[] = {0xFF, 0xFE};
-static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
-static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+static const char utf16_be_bom[] = {'\xFE', '\xFF'};
+static const char utf16_le_bom[] = {'\xFF', '\xFE'};
+static const char utf32_be_bom[] = {'\0', '\0', '\xFE', '\xFF'};
+static const char utf32_le_bom[] = {'\xFF', '\xFE', '\0', '\0'};

  int has_prohibited_utf_bom(const char *enc, const char *data, size_t 
len)
  {
Junio C Hamano· Jul 9, 2018, 16:33 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

Beat Bolli <dev+git@drbeat.li> writes:
Show 22 quoted lines
>>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>
>> An alternative approach that might be easier to read (and avoids the
>> confusion arising from our use of (signed) chars for strings pretty
>> much
>> everywhere):
>>
>> #define FE ((char)0xfe)
>> #define FF ((char)0xff)
>>
>> ...
>
> I have tried this first (without the macros, though), and thought
> it looked really ugly. That's why I chose this solution. The usage
> is pretty local and close to function has_bom_prefix().

I found that what you posted was already OK, as has_bom_prefix() appears only locally in this file and that is the only thing that cares about these foo_bom[] constants. Casting the elements in these arrays to (char) type is also fine and not all that ugly, I think, and between the two (but without the macro) I have no strong preference. I wonder if writing them as '\376' and '\377' as old timers would helps the compiler, though.

Beat Bolli· Jul 9, 2018, 17:56 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

On 09.07.18 18:33, Junio C Hamano wrote:
Show 33 quoted lines
> Beat Bolli <dev+git@drbeat.li> writes:
> 
>>>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>>>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>>>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>>>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>>>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>>
>>> An alternative approach that might be easier to read (and avoids the
>>> confusion arising from our use of (signed) chars for strings pretty
>>> much
>>> everywhere):
>>>
>>> #define FE ((char)0xfe)
>>> #define FF ((char)0xff)
>>>
>>> ...
>>
>> I have tried this first (without the macros, though), and thought
>> it looked really ugly. That's why I chose this solution. The usage
>> is pretty local and close to function has_bom_prefix().
> 
> I found that what you posted was already OK, as has_bom_prefix()
> appears only locally in this file and that is the only thing that
> cares about these foo_bom[] constants.  Casting the elements in
> these arrays to (char) type is also fine and not all that ugly,
> I think, and between the two (but without the macro) I have no
> strong preference.  I wonder if writing them as '\376' and '\377'
> as old timers would helps the compiler, though.
> 

Yes, it does, as I found out in https://public-inbox.org/git/e3df2644b59b170e26b2a7c0d3978331@drbeat.li/

But I prefer hex; it's closer to the usual definition of the BOM bytes.
Beat
Junio C Hamano· Jul 9, 2018, 18:18 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

Junio C Hamano <gitster@pobox.com> writes:
Show 32 quoted lines
> Beat Bolli <dev+git@drbeat.li> writes:
>
>>>> -static const char utf16_be_bom[] = {0xFE, 0xFF};
>>>> -static const char utf16_le_bom[] = {0xFF, 0xFE};
>>>> -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>>> -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>>> +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
>>>> +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
>>>> +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
>>>> +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
>>>
>>> An alternative approach that might be easier to read (and avoids the
>>> confusion arising from our use of (signed) chars for strings pretty
>>> much
>>> everywhere):
>>>
>>> #define FE ((char)0xfe)
>>> #define FF ((char)0xff)
>>>
>>> ...
>>
>> I have tried this first (without the macros, though), and thought
>> it looked really ugly. That's why I chose this solution. The usage
>> is pretty local and close to function has_bom_prefix().
>
> I found that what you posted was already OK, as has_bom_prefix()
> appears only locally in this file and that is the only thing that
> cares about these foo_bom[] constants.  Casting the elements in
> these arrays to (char) type is also fine and not all that ugly,
> I think, and between the two (but without the macro) I have no
> strong preference.  I wonder if writing them as '\376' and '\377'
> as old timers would helps the compiler, though.

Heh, I should have read the remainder of my mailbox to learn that you came to the same conclusion in response to Dscho's comment before saying the above. Hex is OK as all our compilers would be capable of gloking '\xff' (there is a rather unusual '\x20' already in builtin/clone.c).

Thanks.
Johannes Schindelin· Jul 9, 2018, 20:04 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 6/6] utf8.c: avoid char overflow

Hi Beat,
On Mon, 9 Jul 2018, Beat Bolli wrote:
Show 46 quoted lines
> Am 09.07.2018 15:14, schrieb Johannes Schindelin:
> > 
> > On Sun, 8 Jul 2018, Beat Bolli wrote:
> > 
> > > In ISO C, char constants must be in the range -128..127. Change the BOM
> > > constants to unsigned char to avoid overflow.
> > > 
> > > Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> > > ---
> > >  utf8.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/utf8.c b/utf8.c
> > > index d55e20c641..833ce00617 100644
> > > --- a/utf8.c
> > > +++ b/utf8.c
> > > @@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int insz,
> > >  #endif
> > > 
> > > static int has_bom_prefix(const char *data, size_t len,
> > > -			  const char *bom, size_t bom_len)
> > > +			  const unsigned char *bom, size_t bom_len)
> > >  {
> > > 	return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
> > >  }
> > > 
> > > -static const char utf16_be_bom[] = {0xFE, 0xFF};
> > > -static const char utf16_le_bom[] = {0xFF, 0xFE};
> > > -static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
> > > -static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
> > > +static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
> > > +static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
> > > +static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
> > > +static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
> > 
> > An alternative approach that might be easier to read (and avoids the
> > confusion arising from our use of (signed) chars for strings pretty much
> > everywhere):
> > 
> > #define FE ((char)0xfe)
> > #define FF ((char)0xff)
> > 
> > ...
> 
> I have tried this first (without the macros, though), and thought it looked
> really ugly.
Yep, I would totally agree that it would be very ugly without the macros.

Which is why I suggested the macros instead, in which case it looks relatively elegant to my eyes.

Ciao, Dscho

Beat Bolli· Jul 8, 2018, 14:43 UTC · re: Beat Bolli · lore

[RFC PATCH 1/6] connect.h: avoid forward declaration of an enum

Include protocol.h to define enum protocol_version.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 connect.h | 2 ++
 1 file changed, 2 insertions(+)
Show changes to connect.h +2 −0
diff --git a/connect.h b/connect.h
index 0e69c6709c..c86f862f2f 100644
--- a/connect.h
+++ b/connect.h
@@ -1,6 +1,8 @@
 #ifndef CONNECT_H
 #define CONNECT_H
 
+#include "protocol.h"   /* for enum protocol_version */
+
 #define CONNECT_VERBOSE       (1u << 0)
 #define CONNECT_DIAG_URL      (1u << 1)
 #define CONNECT_IPV4          (1u << 2)
-- 
2.15.0.rc1.299.gda03b47c3
Beat Bolli· Jul 8, 2018, 14:43 UTC · re: Beat Bolli · lore

[RFC PATCH 5/6] string-list.c: avoid conversion from void * to function pointer

ISO C forbids the conversion of void pointers to function pointers. Introduce a context struct that encapsulates the function pointer.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 string-list.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
Show changes to string-list.c +14 −4
diff --git a/string-list.c b/string-list.c
index a0cf0cfe88..771c455098 100644
--- a/string-list.c
+++ b/string-list.c
@@ -224,18 +224,28 @@ struct string_list_item *string_list_append(struct string_list *list,
 			list->strdup_strings ? xstrdup(string) : (char *)string);
 }
 
+/*
+ * Encapsulate the compare function pointer because ISO C99 forbids
+ * casting from void * to a function pointer and vice versa.
+ */
+struct string_list_sort_ctx
+{
+	compare_strings_fn cmp;
+};
+
 static int cmp_items(const void *a, const void *b, void *ctx)
 {
-	compare_strings_fn cmp = ctx;
+	struct string_list_sort_ctx *sort_ctx = ctx;
 	const struct string_list_item *one = a;
 	const struct string_list_item *two = b;
-	return cmp(one->string, two->string);
+	return sort_ctx->cmp(one->string, two->string);
 }
 
 void string_list_sort(struct string_list *list)
 {
-	QSORT_S(list->items, list->nr, cmp_items,
-		list->cmp ? list->cmp : strcmp);
+	struct string_list_sort_ctx sort_ctx = {list->cmp ? list->cmp : strcmp};
+
+	QSORT_S(list->items, list->nr, cmp_items, &sort_ctx);
 }
 
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
-- 
2.15.0.rc1.299.gda03b47c3
Johannes Schindelin· Jul 9, 2018, 13:40 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 0/6] Compile cleanly in pedantic mode

Hi Beat,
On Sun, 8 Jul 2018, Beat Bolli wrote:
Show 7 quoted lines
> While developing 6aaded550 ("builtin/config: work around an unsized
> array forward declaration", 2018-07-05), I have compiled Git with
> CFLAGS="-std=c99 -pedantic".
> 
> This is an RFC patch series that fixes a few compiler warnings when
> compiling with these options, always assuming that this is a worthwile
> goal.

Thank you, I think it is safe to remove the RFC label: this is really good to have, in order to make it as portable C as possible.

Ciao, Dscho

Junio C Hamano· Jul 9, 2018, 16:25 UTC · re: Beat Bolli · lore

Re: [RFC PATCH 0/6] Compile cleanly in pedantic mode

Beat Bolli <dev+git@drbeat.li> writes:
> While developing 6aaded550 ("builtin/config: work around an unsized
> array forward declaration", 2018-07-05), I have compiled Git with
> CFLAGS="-std=c99 -pedantic".

Nice. I also pretty recently realized that I stopped building with the pedantic option by accident, probably when DEVELOPER build knob was added to the main Makefile, and have been disturbed by these failures.

Beat Bolli· Jul 9, 2018, 19:25 UTC · re: Beat Bolli · lore

[PATCH 0/6] Compile cleanly in pedantic mode

While developing 6aaded550 ("builtin/config: work around an unsized array forward declaration", 2018-07-05), I have compiled Git with CFLAGS="-std=c99 -pedantic".

This series fixes a few compiler warnings when compiling with these options.

Note that all warnings were produced by -pedantic; the C99 standard option by itself didn't cause any of them.

The warnings were:
1) Char arrays initialized from a parenthesized string.
        Suppressed by defining USE_PARENS_AROUND_GETTEXT_N to 0
        globally. This was done just to keep the amount of warnings
        manageable; this series leaves that knob alone. The advantage of
        not mistakenly concatenating two translated strings is greater.
2) connect.h, refs/refs-internal.h: Forward reference to an enum.
        Added two #includes that define the enums. This was already
        (inconclusively) talked about in [0].
3) convert.c: Invalid escape sequence "\e".
        Replaced with "\033".
4) sequencer.c: Empty statements at top level.
        Removed the extra semicolons.
5) string-list.c: Forbidden to cast from void * to a function pointer and
   vice versa.
        Encapsulated the function pointer in a context struct. This is
        controversial because it has a performance impact, namely one
        additional pointer dereference per string comparison. An
        alternative might be to use multiple casts via intptr_t. But
        I'm not sure if this is worth the trouble.
6) utf8.c: overflow of char values.
        Used proper char literals for the BOM constants.
This series has patches for 2) to 6).

Regards, Beat

[0] https://public-inbox.org/git/53ab8626-f862-a732-b369-abeab69a468f@ramsayjones.plus.com/T/
Beat Bolli (6):
  connect.h: avoid forward declaration of an enum
  refs/refs-internal.h: avoid forward declaration of an enum
  convert.c: replace "\e" escapes with "\033".
  sequencer.c: avoid empty statements at top level
  string-list.c: avoid conversion from void * to function pointer
  utf8.c: avoid char overflow
 connect.h            |  2 ++
 convert.c            |  2 +-
 path.h               |  2 +-
 refs/refs-internal.h |  2 ++
 sequencer.c          |  4 ++--
 string-list.c        | 18 ++++++++++++++----
 utf8.c               |  8 ++++----
 7 files changed, 26 insertions(+), 12 deletions(-)
Interdiff from the RFC series:
Show changes to 2 files +6 −6

path.h, utf8.c

diff --git a/path.h b/path.h
index 1ccd0373c9..fc9d3487a0 100644
--- a/path.h
+++ b/path.h
@@ -147,7 +147,7 @@ extern void report_linked_checkout_garbage(void);
 /*
  * You can define a static memoized git path like:
  *
- *    static GIT_PATH_FUNC(git_path_foo, "FOO");
+ *    static GIT_PATH_FUNC(git_path_foo, "FOO")
  *
  * or use one of the global ones below.
  */
diff --git a/utf8.c b/utf8.c
index 833ce00617..982217eec9 100644
--- a/utf8.c
+++ b/utf8.c
@@ -561,15 +561,15 @@ char *reencode_string_len(const char *in, int insz,
 #endif

 static int has_bom_prefix(const char *data, size_t len,
-                         const unsigned char *bom, size_t bom_len)
+                         const char *bom, size_t bom_len)
 {
        return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
 }

-static const unsigned char utf16_be_bom[] = {0xFE, 0xFF};
-static const unsigned char utf16_le_bom[] = {0xFF, 0xFE};
-static const unsigned char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
-static const unsigned char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+static const char utf16_be_bom[] = {'\xFE', '\xFF'};
+static const char utf16_le_bom[] = {'\xFF', '\xFE'};
+static const char utf32_be_bom[] = {'\0', '\0', '\xFE', '\xFF'};
+static const char utf32_le_bom[] = {'\xFF', '\xFE', '\0', '\0'};

 int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)
 {
-- 
2.18.0.203.gfac676dfb9
Beat Bolli· Jul 9, 2018, 20:25 UTC · re: Beat Bolli · lore

Re: [PATCH 0/6] Compile cleanly in pedantic mode

On 09.07.18 21:25, Beat Bolli wrote:
Show 6 quoted lines
> While developing 6aaded550 ("builtin/config: work around an unsized
> array forward declaration", 2018-07-05), I have compiled Git with
> CFLAGS="-std=c99 -pedantic".
> 
> This series fixes a few compiler warnings when compiling with these
> options.

As a small aside, I have also compiled all of Git with -std=c11 using gcc 8.1. This didn't turn up any new warnings, so we're looking pretty future-proof in this regard.

Cheers, Beat

Junio C Hamano· Jul 9, 2018, 21:45 UTC · re: Beat Bolli · lore

Re: [PATCH 0/6] Compile cleanly in pedantic mode

Beat Bolli <dev+git@drbeat.li> writes:
> While developing 6aaded550 ("builtin/config: work around an unsized
> array forward declaration", 2018-07-05), I have compiled Git with
> CFLAGS="-std=c99 -pedantic".
Nicely done.  

With these 6 patches and the USE_PARENCE_AROUND_GETTEXT_N hack, the forward decl of the unsized static array you dealt with separately becomes the only remaining violation in the codebase, which is good.

Will queue.  Thanks.
Beat Bolli· Jul 9, 2018, 21:47 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/6] Compile cleanly in pedantic mode

On 09.07.18 23:45, Junio C Hamano wrote:
Show 13 quoted lines
> Beat Bolli <dev+git@drbeat.li> writes:
> 
>> While developing 6aaded550 ("builtin/config: work around an unsized
>> array forward declaration", 2018-07-05), I have compiled Git with
>> CFLAGS="-std=c99 -pedantic".
> 
> Nicely done.  
> 
> With these 6 patches and the USE_PARENCE_AROUND_GETTEXT_N hack, the
> forward decl of the unsized static array you dealt with separately
> becomes the only remaining violation in the codebase, which is good.
> 
> Will queue.  Thanks.
Thanks!
Beat
Beat Bolli· Jul 10, 2018, 07:34 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/6] Compile cleanly in pedantic mode

Hi Junio
Am 09.07.2018 23:45, schrieb Junio C Hamano:
Show 13 quoted lines
> Beat Bolli <dev+git@drbeat.li> writes:
> 
>> While developing 6aaded550 ("builtin/config: work around an unsized
>> array forward declaration", 2018-07-05), I have compiled Git with
>> CFLAGS="-std=c99 -pedantic".
> 
> Nicely done.
> 
> With these 6 patches and the USE_PARENCE_AROUND_GETTEXT_N hack, the
> forward decl of the unsized static array you dealt with separately
> becomes the only remaining violation in the codebase, which is good.
> 
> Will queue.  Thanks.

Should we add a "pedantic" flag to DEVOPTS that would simplify building pedantically? It would also have to set USE_PARENS_AROUND_GETTEXT_N so as to not overwhelm the developer with too much output.

Beat
Junio C Hamano· Jul 11, 2018, 15:42 UTC · re: Beat Bolli · lore

Re: [PATCH 0/6] Compile cleanly in pedantic mode

Beat Bolli <dev+git@drbeat.li> writes:
> Should we add a "pedantic" flag to DEVOPTS that would simplify
> building pedantically? It would also have to set
> USE_PARENS_AROUND_GETTEXT_N so as to not overwhelm the developer with
> too much output.

That may be something worth discussing before doing; I'd prefer to wait until these 6 patches, plus the unsized static array one you did spearately, graduates to the 'master' branch.

Johannes Schindelin· Jul 12, 2018, 13:25 UTC · re: Junio C Hamano · lore

Re: [PATCH 0/6] Compile cleanly in pedantic mode

Hi,
On Wed, 11 Jul 2018, Junio C Hamano wrote:
Show 10 quoted lines
> Beat Bolli <dev+git@drbeat.li> writes:
> 
> > Should we add a "pedantic" flag to DEVOPTS that would simplify
> > building pedantically? It would also have to set
> > USE_PARENS_AROUND_GETTEXT_N so as to not overwhelm the developer with
> > too much output.
> 
> That may be something worth discussing before doing; I'd prefer to
> wait until these 6 patches, plus the unsized static array one you
> did spearately, graduates to the 'master' branch.

If this change to DEVOPTS was done as 7/7, then this would be very easy to guarantee (or as 2/2 if you want to graduate this here patch series faster than the static array forward-declaration fix).

Just an idea, Dscho

Junio C Hamano· Jul 12, 2018, 15:40 UTC · re: Johannes Schindelin · lore

Re: [PATCH 0/6] Compile cleanly in pedantic mode

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 7 quoted lines
>> That may be something worth discussing before doing; I'd prefer to
>> wait until these 6 patches, plus the unsized static array one you
>> did spearately, graduates to the 'master' branch.
>
> If this change to DEVOPTS was done as 7/7, then this would be very easy to
> guarantee (or as 2/2 if you want to graduate this here patch series faster
> than the static array forward-declaration fix).

It would guarantee that there is *no* period to discuss if it is too inconvenient for developers between the time these 6 plus 1 graduate to the master branch (and willing volunteers have chance to try it themselves) and the change actuallly is forced upon the developers.

That is why I said I'd prefer to wait.

A separate patch that depends on the 6 plus 1 topic that is kept in 'next' longer than the main series is a possibility, though.

Beat Bolli· Jul 9, 2018, 19:25 UTC · re: Beat Bolli · lore

[PATCH 1/6] connect.h: avoid forward declaration of an enum

Include protocol.h to define enum protocol_version.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 connect.h | 2 ++
 1 file changed, 2 insertions(+)
Show changes to connect.h +2 −0
diff --git a/connect.h b/connect.h
index 0e69c6709c..c86f862f2f 100644
--- a/connect.h
+++ b/connect.h
@@ -1,6 +1,8 @@
 #ifndef CONNECT_H
 #define CONNECT_H
 
+#include "protocol.h"   /* for enum protocol_version */
+
 #define CONNECT_VERBOSE       (1u << 0)
 #define CONNECT_DIAG_URL      (1u << 1)
 #define CONNECT_IPV4          (1u << 2)
-- 
2.18.0.203.gfac676dfb9
Beat Bolli· Jul 9, 2018, 19:25 UTC · re: Beat Bolli · lore

[PATCH 5/6] string-list.c: avoid conversion from void * to function pointer

ISO C forbids the conversion of void pointers to function pointers. Introduce a context struct that encapsulates the function pointer.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 string-list.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)
Show changes to string-list.c +14 −4
diff --git a/string-list.c b/string-list.c
index a0cf0cfe88..771c455098 100644
--- a/string-list.c
+++ b/string-list.c
@@ -224,18 +224,28 @@ struct string_list_item *string_list_append(struct string_list *list,
 			list->strdup_strings ? xstrdup(string) : (char *)string);
 }
 
+/*
+ * Encapsulate the compare function pointer because ISO C99 forbids
+ * casting from void * to a function pointer and vice versa.
+ */
+struct string_list_sort_ctx
+{
+	compare_strings_fn cmp;
+};
+
 static int cmp_items(const void *a, const void *b, void *ctx)
 {
-	compare_strings_fn cmp = ctx;
+	struct string_list_sort_ctx *sort_ctx = ctx;
 	const struct string_list_item *one = a;
 	const struct string_list_item *two = b;
-	return cmp(one->string, two->string);
+	return sort_ctx->cmp(one->string, two->string);
 }
 
 void string_list_sort(struct string_list *list)
 {
-	QSORT_S(list->items, list->nr, cmp_items,
-		list->cmp ? list->cmp : strcmp);
+	struct string_list_sort_ctx sort_ctx = {list->cmp ? list->cmp : strcmp};
+
+	QSORT_S(list->items, list->nr, cmp_items, &sort_ctx);
 }
 
 struct string_list_item *unsorted_string_list_lookup(struct string_list *list,
-- 
2.18.0.203.gfac676dfb9
Beat Bolli· Jul 9, 2018, 19:25 UTC · re: Beat Bolli · lore

[PATCH 3/6] convert.c: replace "\e" escapes with "\033".

The "\e" escape is not defined in ISO C.
While on this line, add a missing space after the comma.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 convert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to convert.c +1 −1
diff --git a/convert.c b/convert.c
index 64d0d30e08..edebb946f5 100644
--- a/convert.c
+++ b/convert.c
@@ -334,7 +334,7 @@ static void trace_encoding(const char *context, const char *path,
 	strbuf_addf(&trace, "%s (%s, considered %s):\n", context, path, encoding);
 	for (i = 0; i < len && buf; ++i) {
 		strbuf_addf(
-			&trace,"| \e[2m%2i:\e[0m %2x \e[2m%c\e[0m%c",
+			&trace, "| \033[2m%2i:\033[0m %2x \033[2m%c\033[0m%c",
 			i,
 			(unsigned char) buf[i],
 			(buf[i] > 32 && buf[i] < 127 ? buf[i] : ' '),
-- 
2.18.0.203.gfac676dfb9
Beat Bolli· Jul 9, 2018, 19:25 UTC · re: Beat Bolli · lore

[PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum

Include iterator.h to define enum iterator_selection.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 refs/refs-internal.h | 2 ++
 1 file changed, 2 insertions(+)
Show changes to refs/refs-internal.h +2 −0
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index dd834314bd..a78b5cb803 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -1,6 +1,8 @@
 #ifndef REFS_REFS_INTERNAL_H
 #define REFS_REFS_INTERNAL_H
 
+#include "iterator.h"   /* for enum iterator_selection */
+
 /*
  * Data structures and functions for the internal use of the refs
  * module. Code outside of the refs module should use only the public
-- 
2.18.0.203.gfac676dfb9
Beat Bolli· Jul 9, 2018, 19:25 UTC · re: Beat Bolli · lore

[PATCH 6/6] utf8.c: avoid char overflow

In ISO C, char constants must be in the range -128..127. Change the BOM constants to char literals to avoid overflow.

Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 utf8.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to utf8.c +4 −4
diff --git a/utf8.c b/utf8.c
index d55e20c641..982217eec9 100644
--- a/utf8.c
+++ b/utf8.c
@@ -566,10 +566,10 @@ static int has_bom_prefix(const char *data, size_t len,
 	return data && bom && (len >= bom_len) && !memcmp(data, bom, bom_len);
 }
 
-static const char utf16_be_bom[] = {0xFE, 0xFF};
-static const char utf16_le_bom[] = {0xFF, 0xFE};
-static const char utf32_be_bom[] = {0x00, 0x00, 0xFE, 0xFF};
-static const char utf32_le_bom[] = {0xFF, 0xFE, 0x00, 0x00};
+static const char utf16_be_bom[] = {'\xFE', '\xFF'};
+static const char utf16_le_bom[] = {'\xFF', '\xFE'};
+static const char utf32_be_bom[] = {'\0', '\0', '\xFE', '\xFF'};
+static const char utf32_le_bom[] = {'\xFF', '\xFE', '\0', '\0'};
 
 int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)
 {
-- 
2.18.0.203.gfac676dfb9
Beat Bolli· Jul 9, 2018, 19:25 UTC · re: Beat Bolli · lore

[PATCH 4/6] sequencer.c: avoid empty statements at top level

The macro GIT_PATH_FUNC expands to a function definition that ends with a closing brace. Remove two extra semicolons.

While at it, fix the example in path.h.
Signed-off-by: Beat Bolli <dev+git@drbeat.li>
---
 path.h      | 2 +-
 sequencer.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
Show changes to 2 files +3 −3

path.h, sequencer.c

diff --git a/path.h b/path.h
index 1ccd0373c9..fc9d3487a0 100644
--- a/path.h
+++ b/path.h
@@ -147,7 +147,7 @@ extern void report_linked_checkout_garbage(void);
 /*
  * You can define a static memoized git path like:
  *
- *    static GIT_PATH_FUNC(git_path_foo, "FOO");
+ *    static GIT_PATH_FUNC(git_path_foo, "FOO")
  *
  * or use one of the global ones below.
  */
diff --git a/sequencer.c b/sequencer.c
index 5354d4d51e..66e7073995 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -62,12 +62,12 @@ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
  * The file to keep track of how many commands were already processed (e.g.
  * for the prompt).
  */
-static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
+static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum")
 /*
  * The file to keep track of how many commands are to be processed in total
  * (e.g. for the prompt).
  */
-static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
+static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end")
 /*
  * The commit message that is planned to be used for any changes that
  * need to be committed following a user interaction.
-- 
2.18.0.203.gfac676dfb9
Junio C Hamano· Jul 9, 2018, 21:37 UTC · re: Beat Bolli · lore

Re: [PATCH 4/6] sequencer.c: avoid empty statements at top level

Beat Bolli <dev+git@drbeat.li> writes:
> The macro GIT_PATH_FUNC expands to a function definition that ends with
> a closing brace. Remove two extra semicolons.
Good.  Thanks.
Show 41 quoted lines
>
> While at it, fix the example in path.h.
>
> Signed-off-by: Beat Bolli <dev+git@drbeat.li>
> ---
>  path.h      | 2 +-
>  sequencer.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/path.h b/path.h
> index 1ccd0373c9..fc9d3487a0 100644
> --- a/path.h
> +++ b/path.h
> @@ -147,7 +147,7 @@ extern void report_linked_checkout_garbage(void);
>  /*
>   * You can define a static memoized git path like:
>   *
> - *    static GIT_PATH_FUNC(git_path_foo, "FOO");
> + *    static GIT_PATH_FUNC(git_path_foo, "FOO")
>   *
>   * or use one of the global ones below.
>   */
> diff --git a/sequencer.c b/sequencer.c
> index 5354d4d51e..66e7073995 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -62,12 +62,12 @@ static GIT_PATH_FUNC(rebase_path_done, "rebase-merge/done")
>   * The file to keep track of how many commands were already processed (e.g.
>   * for the prompt).
>   */
> -static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum");
> +static GIT_PATH_FUNC(rebase_path_msgnum, "rebase-merge/msgnum")
>  /*
>   * The file to keep track of how many commands are to be processed in total
>   * (e.g. for the prompt).
>   */
> -static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end");
> +static GIT_PATH_FUNC(rebase_path_msgtotal, "rebase-merge/end")
>  /*
>   * The commit message that is planned to be used for any changes that
>   * need to be committed following a user interaction.

← back to recent threads