Compile cleanly in pedantic mode
cover 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 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
RFC PATCH 1/6 connect.h: avoid forward declaration of an enum
Include protocol.h to define enum protocol_version.
--- connect.h | 2 ++ 1 file changed, 2 insertions(+)
connect.h +2 -0
@@ -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)
Signature · 1 line
2.15.0.rc1.299.gda03b47c3
PATCH 1/6 connect.h: avoid forward declaration of an enum
In reply to Beat BolliInclude protocol.h to define enum protocol_version.
--- connect.h | 2 ++ 1 file changed, 2 insertions(+)
connect.h +2 -0
@@ -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)
Signature · 1 line
2.18.0.203.gfac676dfb9
RFC PATCH 2/6 refs/refs-internal.h: avoid forward declaration of an enum
In reply to Beat BolliInclude iterator.h to define enum iterator_selection.
--- refs/refs-internal.h | 2 ++ 1 file changed, 2 insertions(+)
refs/refs-internal.h +2 -0
@@ -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
Signature · 1 line
2.15.0.rc1.299.gda03b47c3
Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum
refs/refs-internal.h line 4
Quoted 9 lines · Beat Bolli
refs/refs-internal.h +1
-0@@ -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
Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum
Quoted 22 lines · Jeff King
refs/refs-internal.h line 4
Quoted 9 lines · Beat Bolli
refs/refs-internal.h +1
-0@@ -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. :)
Quoted 8 lines
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
Re: [RFC PATCH 2/6] refs/refs-internal.h: avoid forward declaration of an enum
Quoted 7 lines · Beat Bolli
Quoted 2 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
PATCH 2/6 refs/refs-internal.h: avoid forward declaration of an enum
In reply to Beat BolliInclude iterator.h to define enum iterator_selection.
--- refs/refs-internal.h | 2 ++ 1 file changed, 2 insertions(+)
refs/refs-internal.h +2 -0
@@ -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
Signature · 1 line
2.18.0.203.gfac676dfb9
RFC PATCH 3/6 convert.c: replace "\e" escapes with "\033".
In reply to Beat BolliThe "\e" escape is not defined in ISO C.
While on this line, add a missing space after the comma.
--- convert.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
convert.c +1 -1
@@ -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] : ' '),
Signature · 1 line
2.15.0.rc1.299.gda03b47c3
PATCH 3/6 convert.c: replace "\e" escapes with "\033".
In reply to Beat BolliThe "\e" escape is not defined in ISO C.
While on this line, add a missing space after the comma.
--- convert.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
convert.c +1 -1
@@ -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] : ' '),
Signature · 1 line
2.18.0.203.gfac676dfb9
Hi Beat,
Quoted 7 lines · 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.
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
Quoted 3 lines · 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".
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.
cover Compile cleanly in pedantic mode
In reply to Beat BolliWhile 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/[email protected]/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:
path.h +1 -1
@@ -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. */
utf8.c +5 -5
@@ -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) {
Signature · 1 line
2.18.0.203.gfac676dfb9
Quoted 6 lines · 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.
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
Quoted 3 lines · 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".
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.
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · 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".
Quoted 7 lines
Nicely done.
With these 6 patches and the
USE_PARENCE_AROUND_GETTEXT_Nhack, 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
Hi Junio
Am 09.07.2018 23:45, schrieb Junio C Hamano:
Quoted 6 lines
Quoted 3 lines · 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".
Quoted 7 lines
Nicely done.
With these 6 patches and the
USE_PARENCE_AROUND_GETTEXT_Nhack, 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
Quoted 4 lines · Beat Bolli
Should we add a "pedantic" flag to DEVOPTS that would simplify building pedantically? It would also have to set
USE_PARENS_AROUND_GETTEXT_Nso 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.
Hi,
Quoted 10 lines · Junio C Hamano
Quoted 4 lines · Beat Bolli
Should we add a "pedantic" flag to DEVOPTS that would simplify building pedantically? It would also have to set
USE_PARENS_AROUND_GETTEXT_Nso 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
Quoted 7 lines · Johannes Schindelin
Quoted 3 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.
RFC PATCH 4/6 sequencer.c: avoid empty statements at top level
In reply to Beat BolliThe marco GIT_PATH_FUNC expands to a complete statement including the semicolon. Remove two extra trailing semicolons.
--- sequencer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
sequencer.c +2 -2
@@ -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). */ -staticGIT_PATH_FUNC (rebase_path_msgtotal,"rebase-merge/end" ); +staticGIT_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.
Signature · 1 line
2.15.0.rc1.299.gda03b47c3
Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
Quoted 7 lines · Beat Bolli
The marco
GIT_PATH_FUNCexpands to a complete statement including the semicolon. Remove two extra trailing semicolons.Signed-off-by Beat Bolli <[email protected]>--- 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.
*/
Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
From: "Eric Sunshine" <[email protected]>
To: "Beat Bolli" <[email protected]>
Quoted 2 lines · Beat Bolli
Quoted 1 line
The marco
GIT_PATH_FUNCexpands to a complete statement including the
s/marco/macro/
Quoted 18 lines
Quoted 6 lines
semicolon. Remove two extra trailing semicolons.
Signed-off-by Beat Bolli <[email protected]>--- 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.hwhich teaches the "wrong" way:/*
* You can define a static memoized git path like:
*
* staticGIT_PATH_FUNC(git_path_foo, "FOO");
*
* or use one of the global ones below.
*/
Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
Am 08.07.2018 23:17, schrieb Philip Oakley:
Quoted 6 lines
From: "Eric Sunshine" <[email protected]>
To: "Beat Bolli" <[email protected]>Quoted 2 lines · Beat Bolli
Quoted 1 line
The marco
GIT_PATH_FUNCexpands to a complete statement including thes/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.
Quoted 10 lines
Quoted 10 lines
Quoted 6 lines
semicolon. Remove two extra trailing semicolons.
Signed-off-by Beat Bolli <[email protected]>--- 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.hwhich teaches the "wrong" way:
Quoted 7 lines
Quoted 7 lines
/*
* You can define a static memoized git path like:
*
* staticGIT_PATH_FUNC(git_path_foo, "FOO");
*
* or use one of the global ones below.
*/
Thanks,
Beat
Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
In reply to Beat BolliQuoted 2 lines · Beat Bolli
The marco
GIT_PATH_FUNCexpands 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.
Quoted 25 lines
Signed-off-by Beat Bolli <[email protected]>--- sequencer.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)sequencer.c +2
-2@@ -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). */ -staticGIT_PATH_FUNC (rebase_path_msgtotal,"rebase-merge/end" ); +staticGIT_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.
Re: [RFC PATCH 4/6] sequencer.c: avoid empty statements at top level
Quoted 17 lines · Junio C Hamano
Quoted 2 lines · Beat Bolli
The marco
GIT_PATH_FUNCexpands 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; \ }
Quoted 5 lines
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
PATCH 4/6 sequencer.c: avoid empty statements at top level
In reply to Beat BolliThe 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.
--- path.h | 2 +- sequencer.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
path.h +1 -1
@@ -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. */
sequencer.c +2 -2
@@ -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). */ -staticGIT_PATH_FUNC (rebase_path_msgtotal,"rebase-merge/end" ); +staticGIT_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.
Signature · 1 line
2.18.0.203.gfac676dfb9
Re: [PATCH 4/6] sequencer.c: avoid empty statements at top level
Quoted 2 lines · Beat Bolli
The macro
GIT_PATH_FUNCexpands to a function definition that ends with a closing brace. Remove two extra semicolons.
Good. Thanks.
Quoted 41 lines
While at it, fix the example in
path.h.Signed-off-by Beat Bolli <[email protected]>--- path.h | 2 +- sequencer.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)path.h +1
-1@@ -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. */ sequencer.c +2
-2@@ -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). */ -staticGIT_PATH_FUNC (rebase_path_msgtotal,"rebase-merge/end" ); +staticGIT_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.
RFC PATCH 5/6 string-list.c: avoid conversion from void * to function pointer
In reply to Beat BolliISO C forbids the conversion of void pointers to function pointers. Introduce a context struct that encapsulates the function pointer.
--- string-list.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
string-list.c +14 -4
@@ -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,
Signature · 1 line
2.15.0.rc1.299.gda03b47c3
PATCH 5/6 string-list.c: avoid conversion from void * to function pointer
In reply to Beat BolliISO C forbids the conversion of void pointers to function pointers. Introduce a context struct that encapsulates the function pointer.
--- string-list.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-)
string-list.c +14 -4
@@ -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,
Signature · 1 line
2.18.0.203.gfac676dfb9
RFC PATCH 6/6 utf8.c: avoid char overflow
In reply to Beat BolliIn ISO C, char constants must be in the range -128..127. Change the BOM constants to unsigned char to avoid overflow.
--- utf8.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
utf8.c +5 -5
@@ -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) {
Signature · 1 line
2.15.0.rc1.299.gda03b47c3
Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
Hi Beat,
Quoted 20 lines · 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 <[email protected]>--- utf8.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)utf8.c +1
-1@@ -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);
utf8.c char *reencode_string_len(const char *in, int insz, · line 572
Quoted 10 lines
utf8.c +4
-4} -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
Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
Hi Dscho
Am 09.07.2018 15:14, schrieb Johannes Schindelin:
Quoted 41 lines
Hi Beat,
Quoted 22 lines · 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 <[email protected]>--- utf8.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)utf8.c +1
-1@@ -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);utf8.c char *reencode_string_len(const char *in, int · line 574
Quoted 10 lines
utf8.c +4
-4} -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):
Quoted 4 lines
#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
Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
Am 09.07.2018 16:48, schrieb Beat Bolli:
Quoted 49 lines
Hi Dscho
Am 09.07.2018 15:14, schrieb Johannes Schindelin:
Quoted 46 lines
Hi Beat,
Quoted 33 lines · 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 <[email protected]>--- utf8.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)utf8.c +5
-5@@ -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 ,Quoted 1 line
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)
Quoted 7 lines
Quoted 1 line
...
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
utf8.c +0 -0
@@ -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)
{
Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
In reply to Beat BolliQuoted 14 lines · Beat Bolli
Quoted 14 lines
utf8.c char *reencode_string_len(const char *in, int insz, · line 572
Quoted 8 lines
utf8.c +4
-4-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):
Quoted 8 lines
Quoted 4 lines
#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.
Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
Quoted 25 lines · Junio C Hamano
Quoted 14 lines · Beat Bolli
Quoted 14 lines
utf8.c char *reencode_string_len(const char *in, int insz, · line 572
Quoted 8 lines
utf8.c +4
-4-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):Quoted 8 lines
Quoted 4 lines
#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().
Quoted 8 lines
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/[email protected]/
But I prefer hex; it's closer to the usual definition of the BOM bytes.
Beat
Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
In reply to Junio C HamanoQuoted 25 lines · Junio C Hamano
Quoted 14 lines · Beat Bolli
Quoted 14 lines
utf8.c char *reencode_string_len(const char *in, int insz, · line 572
Quoted 8 lines
utf8.c +4
-4-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):Quoted 8 lines
Quoted 4 lines
#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().
Quoted 7 lines
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.
Re: [RFC PATCH 6/6] utf8.c: avoid char overflow
In reply to Beat BolliHi Beat,
Quoted 39 lines · Beat Bolli
Am 09.07.2018 15:14, schrieb Johannes Schindelin:
Quoted 38 lines · Beat Bolli
Quoted 21 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 <[email protected]>--- utf8.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)utf8.c +1
-1@@ -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);utf8.c char *reencode_string_len(const char *in, int insz, · line 572
Quoted 10 lines
utf8.c +4
-4} -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):
Quoted 7 lines
Quoted 4 lines
#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
PATCH 6/6 utf8.c: avoid char overflow
In reply to Beat BolliIn ISO C, char constants must be in the range -128..127. Change the BOM constants to char literals to avoid overflow.
--- utf8.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
utf8.c +4 -4
@@ -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) {
Signature · 1 line
2.18.0.203.gfac676dfb9