| Message ID | 2d30dc36-6091-4b47-846f-92d3f4a8b135@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-compat-util: convert skip_{prefix,suffix}{,_mem} to bool | expand |
Hi René On 16/12/2023 10:47, René Scharfe wrote: > Use the data type bool and its values true and false to document the > binary return value of skip_prefix() and friends more explicitly. > > This first use of stdbool.h, introduced with C99, is meant to check > whether there are platforms that claim support for C99, as tested by > 7bc341e21b (git-compat-util: add a test balloon for C99 support, > 2021-12-01), but still lack that header for some reason. > > A fallback based on a wider type, e.g. int, would have to deal with > comparisons somehow to emulate that any non-zero value is true: > > bool b1 = 1; > bool b2 = 2; > if (b1 == b2) puts("This is true."); > > int i1 = 1; > int i2 = 2; > if (i1 == i2) puts("Not printed."); > #define BOOLEQ(a, b) (!(a) == !(b)) > if (BOOLEQ(i1, i2)) puts("This is true."); > > So we'd be better off using bool everywhere without a fallback, if > possible. That's why this patch doesn't include any. Thanks for the comprehensive commit message, I agree that we'd be better off avoiding adding a fallback. The patch looks good, I did wonder if we really need to covert all of these functions for a test-balloon but the patch is still pretty small overall. Best Wishes Phillip > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > git-compat-util.h | 42 ++++++++++++++++++++++-------------------- > 1 file changed, 22 insertions(+), 20 deletions(-) > > diff --git a/git-compat-util.h b/git-compat-util.h > index 3e7a59b5ff..603c97e3b3 100644 > --- a/git-compat-util.h > +++ b/git-compat-util.h > @@ -225,6 +225,7 @@ struct strbuf; > #include <stddef.h> > #include <stdlib.h> > #include <stdarg.h> > +#include <stdbool.h> > #include <string.h> > #ifdef HAVE_STRINGS_H > #include <strings.h> /* for strcasecmp() */ > @@ -684,11 +685,11 @@ report_fn get_warn_routine(void); > void set_die_is_recursing_routine(int (*routine)(void)); > > /* > - * If the string "str" begins with the string found in "prefix", return 1. > + * If the string "str" begins with the string found in "prefix", return true. > * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in > * the string right after the prefix). > * > - * Otherwise, return 0 and leave "out" untouched. > + * Otherwise, return false and leave "out" untouched. > * > * Examples: > * > @@ -699,57 +700,58 @@ void set_die_is_recursing_routine(int (*routine)(void)); > * [skip prefix if present, otherwise use whole string] > * skip_prefix(name, "refs/heads/", &name); > */ > -static inline int skip_prefix(const char *str, const char *prefix, > - const char **out) > +static inline bool skip_prefix(const char *str, const char *prefix, > + const char **out) > { > do { > if (!*prefix) { > *out = str; > - return 1; > + return true; > } > } while (*str++ == *prefix++); > - return 0; > + return false; > } > > /* > * Like skip_prefix, but promises never to read past "len" bytes of the input > * buffer, and returns the remaining number of bytes in "out" via "outlen". > */ > -static inline int skip_prefix_mem(const char *buf, size_t len, > - const char *prefix, > - const char **out, size_t *outlen) > +static inline bool skip_prefix_mem(const char *buf, size_t len, > + const char *prefix, > + const char **out, size_t *outlen) > { > size_t prefix_len = strlen(prefix); > if (prefix_len <= len && !memcmp(buf, prefix, prefix_len)) { > *out = buf + prefix_len; > *outlen = len - prefix_len; > - return 1; > + return true; > } > - return 0; > + return false; > } > > /* > - * If buf ends with suffix, return 1 and subtract the length of the suffix > - * from *len. Otherwise, return 0 and leave *len untouched. > + * If buf ends with suffix, return true and subtract the length of the suffix > + * from *len. Otherwise, return false and leave *len untouched. > */ > -static inline int strip_suffix_mem(const char *buf, size_t *len, > - const char *suffix) > +static inline bool strip_suffix_mem(const char *buf, size_t *len, > + const char *suffix) > { > size_t suflen = strlen(suffix); > if (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen)) > - return 0; > + return false; > *len -= suflen; > - return 1; > + return true; > } > > /* > - * If str ends with suffix, return 1 and set *len to the size of the string > - * without the suffix. Otherwise, return 0 and set *len to the size of the > + * If str ends with suffix, return true and set *len to the size of the string > + * without the suffix. Otherwise, return false and set *len to the size of the > * string. > * > * Note that we do _not_ NUL-terminate str to the new length. > */ > -static inline int strip_suffix(const char *str, const char *suffix, size_t *len) > +static inline bool strip_suffix(const char *str, const char *suffix, > + size_t *len) > { > *len = strlen(str); > return strip_suffix_mem(str, len, suffix); > -- > 2.43.0 >
Phillip Wood <phillip.wood123@gmail.com> writes: > Thanks for the comprehensive commit message, I agree that we'd be > better off avoiding adding a fallback. The patch looks good, I did > wonder if we really need to covert all of these functions for a > test-balloon but the patch is still pretty small overall. I do have to wonder, though, if we want to be a bit more careful than just blindly trusting the platform (i.e. <stdbool.h> might exist and __STDC_VERSION__ may say C99, but under the hood their implementation may be buggy and coerce the result of an assignment of 2 to be different from assigning true). In any case, this is a good starting place. Let's queue it, see what happens, and then think about longer-term plans. Thanks.
Am 18.12.23 um 21:19 schrieb Junio C Hamano: > Phillip Wood <phillip.wood123@gmail.com> writes: > >> Thanks for the comprehensive commit message, I agree that we'd be >> better off avoiding adding a fallback. The patch looks good, I did >> wonder if we really need to covert all of these functions for a >> test-balloon but the patch is still pretty small overall. > > I do have to wonder, though, if we want to be a bit more careful > than just blindly trusting the platform (i.e. <stdbool.h> might > exist and __STDC_VERSION__ may say C99, but under the hood their > implementation may be buggy and coerce the result of an assignment > of 2 to be different from assigning true). We could add a compile-time check like below. I can't decide if this would be prudent or paranoid. It's cheap, though, so perhaps just add this tripwire for non-conforming compilers without making a judgement? René diff --git a/git-compat-util.h b/git-compat-util.h index 603c97e3b3..8212feaa37 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -705,7 +705,7 @@ static inline bool skip_prefix(const char *str, const char *prefix, { do { if (!*prefix) { - *out = str; + *out = str + BUILD_ASSERT_OR_ZERO((bool)1 == (bool)2); return true; } } while (*str++ == *prefix++);
On Sat, Dec 16, 2023 at 11:47:21AM +0100, René Scharfe wrote: > Use the data type bool and its values true and false to document the > binary return value of skip_prefix() and friends more explicitly. > > This first use of stdbool.h, introduced with C99, is meant to check > whether there are platforms that claim support for C99, as tested by > 7bc341e21b (git-compat-util: add a test balloon for C99 support, > 2021-12-01), but still lack that header for some reason. > > A fallback based on a wider type, e.g. int, would have to deal with > comparisons somehow to emulate that any non-zero value is true: > > bool b1 = 1; > bool b2 = 2; > if (b1 == b2) puts("This is true."); > > int i1 = 1; > int i2 = 2; > if (i1 == i2) puts("Not printed."); > #define BOOLEQ(a, b) (!(a) == !(b)) > if (BOOLEQ(i1, i2)) puts("This is true."); > > So we'd be better off using bool everywhere without a fallback, if > possible. That's why this patch doesn't include any. Thanks for putting this together. I agree this is the right spot to end up for now (and that if for whatever reason we find that some platforms can't handle it, we probably should revert and not try the naive fallback). -Peff
diff --git a/git-compat-util.h b/git-compat-util.h index 3e7a59b5ff..603c97e3b3 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -225,6 +225,7 @@ struct strbuf; #include <stddef.h> #include <stdlib.h> #include <stdarg.h> +#include <stdbool.h> #include <string.h> #ifdef HAVE_STRINGS_H #include <strings.h> /* for strcasecmp() */ @@ -684,11 +685,11 @@ report_fn get_warn_routine(void); void set_die_is_recursing_routine(int (*routine)(void)); /* - * If the string "str" begins with the string found in "prefix", return 1. + * If the string "str" begins with the string found in "prefix", return true. * The "out" parameter is set to "str + strlen(prefix)" (i.e., to the point in * the string right after the prefix). * - * Otherwise, return 0 and leave "out" untouched. + * Otherwise, return false and leave "out" untouched. * * Examples: * @@ -699,57 +700,58 @@ void set_die_is_recursing_routine(int (*routine)(void)); * [skip prefix if present, otherwise use whole string] * skip_prefix(name, "refs/heads/", &name); */ -static inline int skip_prefix(const char *str, const char *prefix, - const char **out) +static inline bool skip_prefix(const char *str, const char *prefix, + const char **out) { do { if (!*prefix) { *out = str; - return 1; + return true; } } while (*str++ == *prefix++); - return 0; + return false; } /* * Like skip_prefix, but promises never to read past "len" bytes of the input * buffer, and returns the remaining number of bytes in "out" via "outlen". */ -static inline int skip_prefix_mem(const char *buf, size_t len, - const char *prefix, - const char **out, size_t *outlen) +static inline bool skip_prefix_mem(const char *buf, size_t len, + const char *prefix, + const char **out, size_t *outlen) { size_t prefix_len = strlen(prefix); if (prefix_len <= len && !memcmp(buf, prefix, prefix_len)) { *out = buf + prefix_len; *outlen = len - prefix_len; - return 1; + return true; } - return 0; + return false; } /* - * If buf ends with suffix, return 1 and subtract the length of the suffix - * from *len. Otherwise, return 0 and leave *len untouched. + * If buf ends with suffix, return true and subtract the length of the suffix + * from *len. Otherwise, return false and leave *len untouched. */ -static inline int strip_suffix_mem(const char *buf, size_t *len, - const char *suffix) +static inline bool strip_suffix_mem(const char *buf, size_t *len, + const char *suffix) { size_t suflen = strlen(suffix); if (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen)) - return 0; + return false; *len -= suflen; - return 1; + return true; } /* - * If str ends with suffix, return 1 and set *len to the size of the string - * without the suffix. Otherwise, return 0 and set *len to the size of the + * If str ends with suffix, return true and set *len to the size of the string + * without the suffix. Otherwise, return false and set *len to the size of the * string. * * Note that we do _not_ NUL-terminate str to the new length. */ -static inline int strip_suffix(const char *str, const char *suffix, size_t *len) +static inline bool strip_suffix(const char *str, const char *suffix, + size_t *len) { *len = strlen(str); return strip_suffix_mem(str, len, suffix);
Use the data type bool and its values true and false to document the binary return value of skip_prefix() and friends more explicitly. This first use of stdbool.h, introduced with C99, is meant to check whether there are platforms that claim support for C99, as tested by 7bc341e21b (git-compat-util: add a test balloon for C99 support, 2021-12-01), but still lack that header for some reason. A fallback based on a wider type, e.g. int, would have to deal with comparisons somehow to emulate that any non-zero value is true: bool b1 = 1; bool b2 = 2; if (b1 == b2) puts("This is true."); int i1 = 1; int i2 = 2; if (i1 == i2) puts("Not printed."); #define BOOLEQ(a, b) (!(a) == !(b)) if (BOOLEQ(i1, i2)) puts("This is true."); So we'd be better off using bool everywhere without a fallback, if possible. That's why this patch doesn't include any. Signed-off-by: René Scharfe <l.s.r@web.de> --- git-compat-util.h | 42 ++++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) -- 2.43.0