| Message ID | 3070135eb4b9bd16117e82f1817c112c56a24b55.1716318089.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pack-bitmap: pseudo-merge reachability bitmaps | expand |
On Tue, May 21, 2024 at 03:02:29PM -0400, Taylor Blau wrote: > Future commits will want to parse a floating point value from > configuration, but we have no way to parse such a value prior to this > patch. > > The core of the routine is implemented in git_parse_float(). Unlike > git_parse_unsigned() and git_parse_signed(), however, the function > implemented here only works on type "float", and not related types like > "double", or "long double". > > This is because "double" and "long double" use different functions to > convert from ASCII strings to floating point values (strtod() and > strtold(), respectively). Likewise, there is no pointer type that can > assign to any of these values (except for "void *"), so the only way to > define this trio of functions would be with a macro expansion that is > parameterized over the floating point type and conversion function. I agree it doesn't make sense to support both. But if we have to choose between the two, should we just use "double"? I doubt you need the extra precision for your case, but I also doubt that the speed/storage benefits of "float" would matter. And support for "float" in C is kind of weird. There is no "float" specifier for printf. And according to my copy of strtof(3), until C99 we only had strtod()! Regarding using non-integers at all, I do wonder how much we need them. We've usually stuck to integers in other spots, even if it means a sort of pseudo-fixed-point (e.g., rename scores). Looking ahead, you're using these for the power-series knobs. I guess it would be pretty confusing to try to force integers there. I dunno. Not really an objection, but I just wonder if it was something you considered. -Peff
On Thu, May 23, 2024 at 06:02:25AM -0400, Jeff King wrote: > On Tue, May 21, 2024 at 03:02:29PM -0400, Taylor Blau wrote: > > > Future commits will want to parse a floating point value from > > configuration, but we have no way to parse such a value prior to this > > patch. > > > > The core of the routine is implemented in git_parse_float(). Unlike > > git_parse_unsigned() and git_parse_signed(), however, the function > > implemented here only works on type "float", and not related types like > > "double", or "long double". > > > > This is because "double" and "long double" use different functions to > > convert from ASCII strings to floating point values (strtod() and > > strtold(), respectively). Likewise, there is no pointer type that can > > assign to any of these values (except for "void *"), so the only way to > > define this trio of functions would be with a macro expansion that is > > parameterized over the floating point type and conversion function. > > I agree it doesn't make sense to support both. But if we have to choose > between the two, should we just use "double"? Yeah, I share your feeling that there is no great need to support double-precision floats here, but there's also no reason to artificially limit ourselves to single-precision ones, either. > Regarding using non-integers at all, I do wonder how much we need them. > We've usually stuck to integers in other spots, even if it means a sort > of pseudo-fixed-point (e.g., rename scores). Looking ahead, you're using > these for the power-series knobs. I guess it would be pretty confusing > to try to force integers there. I dunno. Not really an objection, but I > just wonder if it was something you considered. I had originally written the series that way, but Patrick suggested that I use floating point numbers instead ;-). Thanks, Taylor
diff --git a/config.c b/config.c index 77a0fd2d80e..ee681fda34b 100644 --- a/config.c +++ b/config.c @@ -1243,6 +1243,15 @@ ssize_t git_config_ssize_t(const char *name, const char *value, return ret; } +float git_config_float(const char *name, const char *value, + const struct key_value_info *kvi) +{ + float ret; + if (!git_parse_float(value, &ret)) + die_bad_number(name, value, kvi); + return ret; +} + static const struct fsync_component_name { const char *name; enum fsync_component component_bits; diff --git a/config.h b/config.h index f4966e37494..b0d1baba95a 100644 --- a/config.h +++ b/config.h @@ -261,6 +261,12 @@ unsigned long git_config_ulong(const char *, const char *, ssize_t git_config_ssize_t(const char *, const char *, const struct key_value_info *); +/** + * Identical to `git_config_int`, but for floating point values. + */ +float git_config_float(const char *, const char *, + const struct key_value_info *); + /** * Same as `git_config_bool`, except that integers are returned as-is, and * an `is_bool` flag is unset. diff --git a/parse.c b/parse.c index 42d691a0fbb..a5967e80910 100644 --- a/parse.c +++ b/parse.c @@ -125,6 +125,35 @@ int git_parse_ssize_t(const char *value, ssize_t *ret) return 1; } +int git_parse_float(const char *value, float *ret) +{ + char *end; + float val; + uintmax_t factor; + + if (!value || !*value) { + errno = EINVAL; + return 0; + } + + errno = 0; + val = strtof(value, &end); + if (errno == ERANGE) + return 0; + if (end == value) { + errno = EINVAL; + return 0; + } + factor = get_unit_factor(end); + if (!factor) { + errno = EINVAL; + return 0; + } + val *= factor; + *ret = val; + return 1; +} + int git_parse_maybe_bool_text(const char *value) { if (!value) diff --git a/parse.h b/parse.h index 07d2193d698..7df82c5f5b8 100644 --- a/parse.h +++ b/parse.h @@ -6,6 +6,7 @@ int git_parse_ssize_t(const char *, ssize_t *); int git_parse_ulong(const char *, unsigned long *); int git_parse_int(const char *value, int *ret); int git_parse_int64(const char *value, int64_t *ret); +int git_parse_float(const char *value, float *ret); /** * Same as `git_config_bool`, except that it returns -1 on error rather
Future commits will want to parse a floating point value from configuration, but we have no way to parse such a value prior to this patch. The core of the routine is implemented in git_parse_float(). Unlike git_parse_unsigned() and git_parse_signed(), however, the function implemented here only works on type "float", and not related types like "double", or "long double". This is because "double" and "long double" use different functions to convert from ASCII strings to floating point values (strtod() and strtold(), respectively). Likewise, there is no pointer type that can assign to any of these values (except for "void *"), so the only way to define this trio of functions would be with a macro expansion that is parameterized over the floating point type and conversion function. That is all doable, but likely to be overkill given our current needs, which is only to parse floats. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- config.c | 9 +++++++++ config.h | 6 ++++++ parse.c | 29 +++++++++++++++++++++++++++++ parse.h | 1 + 4 files changed, 45 insertions(+)