git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [RFC PATCH 0/6] Compile cleanly in pedantic mode
@ 2018-07-08 14:43 Beat Bolli
  2018-07-08 14:43 ` [RFC PATCH 1/6] connect.h: avoid forward declaration of an enum Beat Bolli
                   ` (14 more replies)
  0 siblings, 15 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-08 14:43 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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/

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [RFC PATCH 1/6] connect.h: avoid forward declaration of an enum
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
@ 2018-07-08 14:43 ` Beat Bolli
  2018-07-08 14:43 ` [RFC PATCH 2/6] refs/refs-internal.h: " Beat Bolli
                   ` (13 subsequent siblings)
  14 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-08 14:43 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
  2018-07-08 14:43 ` [RFC PATCH 1/6] connect.h: avoid forward declaration of an enum Beat Bolli
@ 2018-07-08 14:43 ` " Beat Bolli
  2018-07-09 18:46   ` Jeff King
  2018-07-08 14:43 ` [RFC PATCH 3/6] convert.c: replace "\e" escapes with "\033" Beat Bolli
                   ` (12 subsequent siblings)
  14 siblings, 1 reply; 39+ messages in thread
From: Beat Bolli @ 2018-07-08 14:43 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [RFC PATCH 3/6] convert.c: replace "\e" escapes with "\033".
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
  2018-07-08 14:43 ` [RFC PATCH 1/6] connect.h: avoid forward declaration of an enum Beat Bolli
  2018-07-08 14:43 ` [RFC PATCH 2/6] refs/refs-internal.h: " Beat Bolli
@ 2018-07-08 14:43 ` Beat Bolli
  2018-07-08 14:43 ` [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
                   ` (11 subsequent siblings)
  14 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-08 14:43 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (2 preceding siblings ...)
  2018-07-08 14:43 ` [RFC PATCH 3/6] convert.c: replace "\e" escapes with "\033" Beat Bolli
@ 2018-07-08 14:43 ` Beat Bolli
  2018-07-08 20:54   ` Eric Sunshine
  2018-07-09 21:34   ` Junio C Hamano
  2018-07-08 14:43 ` [RFC PATCH 5/6] string-list.c: avoid conversion from void * to function pointer Beat Bolli
                   ` (10 subsequent siblings)
  14 siblings, 2 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-08 14:43 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [RFC PATCH 5/6] string-list.c: avoid conversion from void * to function pointer
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (3 preceding siblings ...)
  2018-07-08 14:43 ` [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
@ 2018-07-08 14:43 ` Beat Bolli
  2018-07-08 14:43 ` [RFC PATCH 6/6] utf8.c: avoid char overflow Beat Bolli
                   ` (9 subsequent siblings)
  14 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-08 14:43 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [RFC PATCH 6/6] utf8.c: avoid char overflow
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (4 preceding siblings ...)
  2018-07-08 14:43 ` [RFC PATCH 5/6] string-list.c: avoid conversion from void * to function pointer Beat Bolli
@ 2018-07-08 14:43 ` Beat Bolli
  2018-07-09 13:14   ` Johannes Schindelin
  2018-07-09 13:40 ` [RFC PATCH 0/6] Compile cleanly in pedantic mode Johannes Schindelin
                   ` (8 subsequent siblings)
  14 siblings, 1 reply; 39+ messages in thread
From: Beat Bolli @ 2018-07-08 14:43 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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};
 
 int has_prohibited_utf_bom(const char *enc, const char *data, size_t len)
 {
-- 
2.15.0.rc1.299.gda03b47c3


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
  2018-07-08 14:43 ` [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
@ 2018-07-08 20:54   ` Eric Sunshine
  2018-07-08 21:17     ` Philip Oakley
  2018-07-09 21:34   ` Junio C Hamano
  1 sibling, 1 reply; 39+ messages in thread
From: Eric Sunshine @ 2018-07-08 20:54 UTC (permalink / raw)
  To: Beat Bolli; +Cc: Git List, Junio C Hamano

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
  2018-07-08 20:54   ` Eric Sunshine
@ 2018-07-08 21:17     ` Philip Oakley
  2018-07-09  9:37       ` ig
  0 siblings, 1 reply; 39+ messages in thread
From: Philip Oakley @ 2018-07-08 21:17 UTC (permalink / raw)
  To: Eric Sunshine, Beat Bolli; +Cc: Git List, Junio C Hamano

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/

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
  2018-07-08 21:17     ` Philip Oakley
@ 2018-07-09  9:37       ` ig
  0 siblings, 0 replies; 39+ messages in thread
From: ig @ 2018-07-09  9:37 UTC (permalink / raw)
  To: Philip Oakley; +Cc: Eric Sunshine, Beat Bolli, Git List, Junio C Hamano

Am 08.07.2018 23:17, schrieb Philip Oakley:
> 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.

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
  2018-07-08 14:43 ` [RFC PATCH 6/6] utf8.c: avoid char overflow Beat Bolli
@ 2018-07-09 13:14   ` Johannes Schindelin
  2018-07-09 14:48     ` Beat Bolli
  0 siblings, 1 reply; 39+ messages in thread
From: Johannes Schindelin @ 2018-07-09 13:14 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git, gitster

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)

...

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (5 preceding siblings ...)
  2018-07-08 14:43 ` [RFC PATCH 6/6] utf8.c: avoid char overflow Beat Bolli
@ 2018-07-09 13:40 ` Johannes Schindelin
  2018-07-09 16:25 ` Junio C Hamano
                   ` (7 subsequent siblings)
  14 siblings, 0 replies; 39+ messages in thread
From: Johannes Schindelin @ 2018-07-09 13:40 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git, gitster

Hi Beat,

On Sun, 8 Jul 2018, Beat Bolli wrote:

> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
  2018-07-09 13:14   ` Johannes Schindelin
@ 2018-07-09 14:48     ` Beat Bolli
  2018-07-09 15:45       ` Beat Bolli
                         ` (2 more replies)
  0 siblings, 3 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 14:48 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, gitster

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?

Beat

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
  2018-07-09 14:48     ` Beat Bolli
@ 2018-07-09 15:45       ` Beat Bolli
  2018-07-09 16:33       ` Junio C Hamano
  2018-07-09 20:04       ` Johannes Schindelin
  2 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 15:45 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git, gitster

Am 09.07.2018 16:48, schrieb Beat Bolli:
> 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


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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (6 preceding siblings ...)
  2018-07-09 13:40 ` [RFC PATCH 0/6] Compile cleanly in pedantic mode Johannes Schindelin
@ 2018-07-09 16:25 ` Junio C Hamano
  2018-07-09 19:25 ` [PATCH " Beat Bolli
                   ` (6 subsequent siblings)
  14 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2018-07-09 16:25 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git

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.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
  2018-07-09 14:48     ` Beat Bolli
  2018-07-09 15:45       ` Beat Bolli
@ 2018-07-09 16:33       ` Junio C Hamano
  2018-07-09 17:56         ` Beat Bolli
  2018-07-09 18:18         ` Junio C Hamano
  2018-07-09 20:04       ` Johannes Schindelin
  2 siblings, 2 replies; 39+ messages in thread
From: Junio C Hamano @ 2018-07-09 16:33 UTC (permalink / raw)
  To: Beat Bolli; +Cc: Johannes Schindelin, git

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.


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
  2018-07-09 16:33       ` Junio C Hamano
@ 2018-07-09 17:56         ` Beat Bolli
  2018-07-09 18:18         ` Junio C Hamano
  1 sibling, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 17:56 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Johannes Schindelin, git

On 09.07.18 18:33, Junio C Hamano wrote:
> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
  2018-07-09 16:33       ` Junio C Hamano
  2018-07-09 17:56         ` Beat Bolli
@ 2018-07-09 18:18         ` Junio C Hamano
  1 sibling, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2018-07-09 18:18 UTC (permalink / raw)
  To: Beat Bolli; +Cc: Johannes Schindelin, git

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

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



^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum
  2018-07-08 14:43 ` [RFC PATCH 2/6] refs/refs-internal.h: " Beat Bolli
@ 2018-07-09 18:46   ` Jeff King
  2018-07-09 19:30     ` Beat Bolli
  0 siblings, 1 reply; 39+ messages in thread
From: Jeff King @ 2018-07-09 18:46 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git, gitster

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (7 preceding siblings ...)
  2018-07-09 16:25 ` Junio C Hamano
@ 2018-07-09 19:25 ` " Beat Bolli
  2018-07-09 20:25   ` Beat Bolli
  2018-07-09 21:45   ` Junio C Hamano
  2018-07-09 19:25 ` [PATCH 1/6] connect.h: avoid forward declaration of an enum Beat Bolli
                   ` (5 subsequent siblings)
  14 siblings, 2 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 19:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 1/6] connect.h: avoid forward declaration of an enum
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (8 preceding siblings ...)
  2018-07-09 19:25 ` [PATCH " Beat Bolli
@ 2018-07-09 19:25 ` Beat Bolli
  2018-07-09 19:25 ` [PATCH 2/6] refs/refs-internal.h: " Beat Bolli
                   ` (4 subsequent siblings)
  14 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 19:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (9 preceding siblings ...)
  2018-07-09 19:25 ` [PATCH 1/6] connect.h: avoid forward declaration of an enum Beat Bolli
@ 2018-07-09 19:25 ` " Beat Bolli
  2018-07-09 19:25 ` [PATCH 3/6] convert.c: replace "\e" escapes with "\033" Beat Bolli
                   ` (3 subsequent siblings)
  14 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 19:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 3/6] convert.c: replace "\e" escapes with "\033".
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (10 preceding siblings ...)
  2018-07-09 19:25 ` [PATCH 2/6] refs/refs-internal.h: " Beat Bolli
@ 2018-07-09 19:25 ` Beat Bolli
  2018-07-09 19:25 ` [PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
                   ` (2 subsequent siblings)
  14 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 19:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 4/6] sequencer.c: avoid empty statements at top level
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (11 preceding siblings ...)
  2018-07-09 19:25 ` [PATCH 3/6] convert.c: replace "\e" escapes with "\033" Beat Bolli
@ 2018-07-09 19:25 ` Beat Bolli
  2018-07-09 21:37   ` Junio C Hamano
  2018-07-09 19:25 ` [PATCH 5/6] string-list.c: avoid conversion from void * to function pointer Beat Bolli
  2018-07-09 19:25 ` [PATCH 6/6] utf8.c: avoid char overflow Beat Bolli
  14 siblings, 1 reply; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 19:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 5/6] string-list.c: avoid conversion from void * to function pointer
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (12 preceding siblings ...)
  2018-07-09 19:25 ` [PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
@ 2018-07-09 19:25 ` Beat Bolli
  2018-07-09 19:25 ` [PATCH 6/6] utf8.c: avoid char overflow Beat Bolli
  14 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 19:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 6/6] utf8.c: avoid char overflow
  2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
                   ` (13 preceding siblings ...)
  2018-07-09 19:25 ` [PATCH 5/6] string-list.c: avoid conversion from void * to function pointer Beat Bolli
@ 2018-07-09 19:25 ` Beat Bolli
  14 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 19:25 UTC (permalink / raw)
  To: git; +Cc: gitster, Beat Bolli

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

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


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum
  2018-07-09 18:46   ` Jeff King
@ 2018-07-09 19:30     ` Beat Bolli
  2018-07-10  2:15       ` Jeff King
  0 siblings, 1 reply; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 19:30 UTC (permalink / raw)
  To: Jeff King; +Cc: git, gitster

On 09.07.18 20:46, Jeff King wrote:
> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
  2018-07-09 14:48     ` Beat Bolli
  2018-07-09 15:45       ` Beat Bolli
  2018-07-09 16:33       ` Junio C Hamano
@ 2018-07-09 20:04       ` Johannes Schindelin
  2 siblings, 0 replies; 39+ messages in thread
From: Johannes Schindelin @ 2018-07-09 20:04 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git, gitster

Hi Beat,

On Mon, 9 Jul 2018, Beat Bolli wrote:

> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-09 19:25 ` [PATCH " Beat Bolli
@ 2018-07-09 20:25   ` Beat Bolli
  2018-07-09 21:45   ` Junio C Hamano
  1 sibling, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 20:25 UTC (permalink / raw)
  To: git; +Cc: gitster

On 09.07.18 21:25, Beat Bolli wrote:
> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
  2018-07-08 14:43 ` [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
  2018-07-08 20:54   ` Eric Sunshine
@ 2018-07-09 21:34   ` Junio C Hamano
  2018-07-09 21:37     ` Beat Bolli
  1 sibling, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2018-07-09 21:34 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git

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.

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 4/6] sequencer.c: avoid empty statements at top level
  2018-07-09 19:25 ` [PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
@ 2018-07-09 21:37   ` Junio C Hamano
  0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2018-07-09 21:37 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git

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.

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
  2018-07-09 21:34   ` Junio C Hamano
@ 2018-07-09 21:37     ` Beat Bolli
  0 siblings, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 21:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On 09.07.18 23:34, Junio C Hamano wrote:
> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-09 19:25 ` [PATCH " Beat Bolli
  2018-07-09 20:25   ` Beat Bolli
@ 2018-07-09 21:45   ` Junio C Hamano
  2018-07-09 21:47     ` Beat Bolli
  2018-07-10  7:34     ` Beat Bolli
  1 sibling, 2 replies; 39+ messages in thread
From: Junio C Hamano @ 2018-07-09 21:45 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git

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.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-09 21:45   ` Junio C Hamano
@ 2018-07-09 21:47     ` Beat Bolli
  2018-07-10  7:34     ` Beat Bolli
  1 sibling, 0 replies; 39+ messages in thread
From: Beat Bolli @ 2018-07-09 21:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On 09.07.18 23:45, Junio C Hamano wrote:
> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum
  2018-07-09 19:30     ` Beat Bolli
@ 2018-07-10  2:15       ` Jeff King
  0 siblings, 0 replies; 39+ messages in thread
From: Jeff King @ 2018-07-10  2:15 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git, gitster

On Mon, Jul 09, 2018 at 09:30:12PM +0200, Beat Bolli wrote:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-09 21:45   ` Junio C Hamano
  2018-07-09 21:47     ` Beat Bolli
@ 2018-07-10  7:34     ` Beat Bolli
  2018-07-11 15:42       ` Junio C Hamano
  1 sibling, 1 reply; 39+ messages in thread
From: Beat Bolli @ 2018-07-10  7:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Junio C Hamano

Hi Junio

Am 09.07.2018 23:45, schrieb Junio C Hamano:
> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-10  7:34     ` Beat Bolli
@ 2018-07-11 15:42       ` Junio C Hamano
  2018-07-12 13:25         ` Johannes Schindelin
  0 siblings, 1 reply; 39+ messages in thread
From: Junio C Hamano @ 2018-07-11 15:42 UTC (permalink / raw)
  To: Beat Bolli; +Cc: git

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.

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-11 15:42       ` Junio C Hamano
@ 2018-07-12 13:25         ` Johannes Schindelin
  2018-07-12 15:40           ` Junio C Hamano
  0 siblings, 1 reply; 39+ messages in thread
From: Johannes Schindelin @ 2018-07-12 13:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Beat Bolli, git

Hi,

On Wed, 11 Jul 2018, Junio C Hamano wrote:

> 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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 0/6] Compile cleanly in pedantic mode
  2018-07-12 13:25         ` Johannes Schindelin
@ 2018-07-12 15:40           ` Junio C Hamano
  0 siblings, 0 replies; 39+ messages in thread
From: Junio C Hamano @ 2018-07-12 15:40 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Beat Bolli, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

end of thread, back to index

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-08 14:43 [RFC PATCH 0/6] Compile cleanly in pedantic mode Beat Bolli
2018-07-08 14:43 ` [RFC PATCH 1/6] connect.h: avoid forward declaration of an enum Beat Bolli
2018-07-08 14:43 ` [RFC PATCH 2/6] refs/refs-internal.h: " Beat Bolli
2018-07-09 18:46   ` Jeff King
2018-07-09 19:30     ` Beat Bolli
2018-07-10  2:15       ` Jeff King
2018-07-08 14:43 ` [RFC PATCH 3/6] convert.c: replace "\e" escapes with "\033" Beat Bolli
2018-07-08 14:43 ` [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
2018-07-08 20:54   ` Eric Sunshine
2018-07-08 21:17     ` Philip Oakley
2018-07-09  9:37       ` ig
2018-07-09 21:34   ` Junio C Hamano
2018-07-09 21:37     ` Beat Bolli
2018-07-08 14:43 ` [RFC PATCH 5/6] string-list.c: avoid conversion from void * to function pointer Beat Bolli
2018-07-08 14:43 ` [RFC PATCH 6/6] utf8.c: avoid char overflow Beat Bolli
2018-07-09 13:14   ` Johannes Schindelin
2018-07-09 14:48     ` Beat Bolli
2018-07-09 15:45       ` Beat Bolli
2018-07-09 16:33       ` Junio C Hamano
2018-07-09 17:56         ` Beat Bolli
2018-07-09 18:18         ` Junio C Hamano
2018-07-09 20:04       ` Johannes Schindelin
2018-07-09 13:40 ` [RFC PATCH 0/6] Compile cleanly in pedantic mode Johannes Schindelin
2018-07-09 16:25 ` Junio C Hamano
2018-07-09 19:25 ` [PATCH " Beat Bolli
2018-07-09 20:25   ` Beat Bolli
2018-07-09 21:45   ` Junio C Hamano
2018-07-09 21:47     ` Beat Bolli
2018-07-10  7:34     ` Beat Bolli
2018-07-11 15:42       ` Junio C Hamano
2018-07-12 13:25         ` Johannes Schindelin
2018-07-12 15:40           ` Junio C Hamano
2018-07-09 19:25 ` [PATCH 1/6] connect.h: avoid forward declaration of an enum Beat Bolli
2018-07-09 19:25 ` [PATCH 2/6] refs/refs-internal.h: " Beat Bolli
2018-07-09 19:25 ` [PATCH 3/6] convert.c: replace "\e" escapes with "\033" Beat Bolli
2018-07-09 19:25 ` [PATCH 4/6] sequencer.c: avoid empty statements at top level Beat Bolli
2018-07-09 21:37   ` Junio C Hamano
2018-07-09 19:25 ` [PATCH 5/6] string-list.c: avoid conversion from void * to function pointer Beat Bolli
2018-07-09 19:25 ` [PATCH 6/6] utf8.c: avoid char overflow Beat Bolli

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox