Skip to content

Draft: Replace MPFR with GMP-only solution #161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tkfu wants to merge 1 commit into from
+87 −46
Closed

Draft: Replace MPFR with GMP-only solution #161

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ LIBBYTESIZE_PKG_CHECK_MODULES([PCRE2], [libpcre2-8])

AC_CHECK_LIB(gmp, __gmpz_init)

AC_CHECK_HEADERS([langinfo.h gmp.h mpfr.h stdint.h stdbool.h stdarg.h string.h stdio.h ctype.h],
AC_CHECK_HEADERS([langinfo.h gmp.h stdint.h stdbool.h stdarg.h string.h stdio.h ctype.h],
[],
[LIBBYTESIZE_SOFT_FAILURE([Header file $ac_header not found.])],
[])
Expand Down
1 change: 0 additions & 1 deletion dist/libbytesize.spec.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ Source0: https://github.com/storaged-project/libbytesize/releases/download/%
BuildRequires: make
BuildRequires: gcc
BuildRequires: gmp-devel
BuildRequires: mpfr-devel
BuildRequires: pcre2-devel
BuildRequires: gettext-devel
%if %{with_python3}
Expand Down
36 changes: 18 additions & 18 deletions po/libbytesize.pot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ msgid ""
msgstr ""
"Project-Id-Version: libbytesize 2.12\n"
"Report-Msgid-Bugs-To: [email protected]\n"
"POT-Creation-Date: 2025-12-04 12:49+0100\n"
"POT-Creation-Date: 2025-12-17 12:02-0800\n"
"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
"Language-Team: LANGUAGE <[email protected]>\n"
Expand All @@ -18,86 +18,86 @@ msgstr ""
"Content-Transfer-Encoding: 8bit\n"

#. TRANSLATORS: 'B' for bytes
#: src/bs_size.c:52 src/bs_size.c:73
#: src/bs_size.c:51 src/bs_size.c:72
msgid "B"
msgstr ""

#. TRANSLATORS: abbreviation for kibibyte, 2**10 bytes
#: src/bs_size.c:54
#: src/bs_size.c:53
msgid "KiB"
msgstr ""

#. TRANSLATORS: abbreviation for mebibyte, 2**20 bytes
#: src/bs_size.c:56
#: src/bs_size.c:55
msgid "MiB"
msgstr ""

#. TRANSLATORS: abbreviation for gibibyte, 2**30 bytes
#: src/bs_size.c:58
#: src/bs_size.c:57
msgid "GiB"
msgstr ""

#. TRANSLATORS: abbreviation for tebibyte, 2**40 bytes
#: src/bs_size.c:60
#: src/bs_size.c:59
msgid "TiB"
msgstr ""

#. TRANSLATORS: abbreviation for pebibyte, 2**50 bytes
#: src/bs_size.c:62
#: src/bs_size.c:61
msgid "PiB"
msgstr ""

#. TRANSLATORS: abbreviation for exbibyte, 2**60 bytes
#: src/bs_size.c:64
#: src/bs_size.c:63
msgid "EiB"
msgstr ""

#. TRANSLATORS: abbreviation for zebibyte, 2**70 bytes
#: src/bs_size.c:66
#: src/bs_size.c:65
msgid "ZiB"
msgstr ""

#. TRANSLATORS: abbreviation for yobibyte, 2**80 bytes
#: src/bs_size.c:68
#: src/bs_size.c:67
msgid "YiB"
msgstr ""

#. TRANSLATORS: abbreviation for kilobyte, 10**3 bytes
#: src/bs_size.c:75
#: src/bs_size.c:74
msgid "KB"
msgstr ""

#. TRANSLATORS: abbreviation for megabyte, 10**6 bytes
#: src/bs_size.c:77
#: src/bs_size.c:76
msgid "MB"
msgstr ""

#. TRANSLATORS: abbreviation for gigabyte, 10**9 bytes
#: src/bs_size.c:79
#: src/bs_size.c:78
msgid "GB"
msgstr ""

#. TRANSLATORS: abbreviation for terabyte, 10**12 bytes
#: src/bs_size.c:81
#: src/bs_size.c:80
msgid "TB"
msgstr ""

#. TRANSLATORS: abbreviation for petabyte, 10**15 bytes
#: src/bs_size.c:83
#: src/bs_size.c:82
msgid "PB"
msgstr ""

#. TRANSLATORS: abbreviation for exabyte, 10**18 bytes
#: src/bs_size.c:85
#: src/bs_size.c:84
msgid "EB"
msgstr ""

#. TRANSLATORS: abbreviation for zettabyte, 10**21 bytes
#: src/bs_size.c:87
#: src/bs_size.c:86
msgid "ZB"
msgstr ""

#. TRANSLATORS: abbreviation for yottabyte, 10**24 bytes
#: src/bs_size.c:89
#: src/bs_size.c:88
msgid "YB"
msgstr ""
2 changes: 1 addition & 1 deletion src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ LDADD = $(LIBINTL)

lib_LTLIBRARIES = libbytesize.la
libbytesize_la_CFLAGS = -Wall -Wextra -Werror -Wno-overflow -D_GNU_SOURCE
libbytesize_la_LIBADD = -lgmp -lmpfr $(PCRE2_LIBS)
libbytesize_la_LIBADD = -lgmp $(PCRE2_LIBS)
libbytesize_la_LDFLAGS = -version-info 1:0:0
libbytesize_la_SOURCES = bs_size.c bs_size.h gettext.h

Expand Down
90 changes: 66 additions & 24 deletions src/bs_size.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#include <gmp.h>
#include <mpfr.h>
#include <langinfo.h>
#include <stdarg.h>
#include <stdio.h>
Expand Down Expand Up @@ -224,30 +223,66 @@ static void strstrip(char *str) {
str[i-begin] = '\0';
}

static bool multiply_size_by_unit (mpfr_t size, char *unit_str) {
/* Helper function to convert mpf_t to mpz_t with round-toward-zero (truncate decimal) */
static void mpz_set_f_round_zero(mpz_t rop, const mpf_t op) {
Copy link

@PlasmaPower PlasmaPower Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't libgmp already provide mpz_set_f which does this exact thing? https://gmplib.org/manual/Assigning-Integers

Relying on the formatting and parsing seems hacky.

Copy link
Author

@tkfu tkfu Dec 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because libgmp's mpz_set_f doesn't handle the decimal truncation properly (edit: I guess I should say, it doesn't handle truncation the way we would like it to here. it's intended behaviour for libgmp to truncate the binary representation). As noted in the commit introducing mpfr, mpz_set_f(0.1 * 1000) is 99.

Agree that it's a bit hacky, though.

Copy link

@PlasmaPower PlasmaPower Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure the string based method is different? Is there a test case that fails with mpz_set_f? It seems to me the issue with the 0.1 * 1000 case is not the truncation but the result of the multiplication itself.

Copy link
Author

@tkfu tkfu Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.....you're right. Damn, back to the drawing board. Thank you for the careful review. I just assumed the test cases had coverage for this without verifying, and they definitely don't.

char *str;
const char *radix_char;
char *dot;
char *exp;

/* Convert to decimal string with enough precision */
gmp_asprintf(&str, "%.100Ff", op);

/* Get the locale's decimal separator */
radix_char = nl_langinfo(RADIXCHAR);

/* Find decimal point (using locale's separator) and truncate */
dot = strchr(str, radix_char[0]);
if (dot) {
*dot = '\0';
}

/* Remove any exponent notation */
exp = strchr(str, 'e');
if (!exp) exp = strchr(str, 'E');
if (exp) *exp = '\0';

mpz_set_str(rop, str, 10);
free(str);
}

static bool multiply_size_by_unit (mpf_t size, char *unit_str) {
BSBunit bunit = BS_BUNIT_UNDEF;
BSDunit dunit = BS_DUNIT_UNDEF;
uint64_t pwr = 0;
mpfr_t dec_mul;
mpf_t dec_mul;
mpf_t base;
size_t unit_str_len = 0;
uint64_t i;

unit_str_len = strlen (unit_str);

for (bunit=BS_BUNIT_B; bunit < BS_BUNIT_UNDEF; bunit++)
if (strncasecmp (unit_str, b_units[bunit-BS_BUNIT_B], unit_str_len) == 0) {
pwr = (uint64_t) bunit - BS_BUNIT_B;
mpfr_mul_2exp (size, size, 10 * pwr, MPFR_RNDN);
mpf_mul_2exp (size, size, 10 * pwr);
return true;
}

mpfr_init2 (dec_mul, BS_FLOAT_PREC_BITS);
mpfr_set_ui (dec_mul, 1000, MPFR_RNDN);
mpf_init2 (dec_mul, BS_FLOAT_PREC_BITS);
mpf_init2 (base, BS_FLOAT_PREC_BITS);
mpf_set_ui (base, 1000);
for (dunit=BS_DUNIT_B; dunit < BS_DUNIT_UNDEF; dunit++)
if (strncasecmp (unit_str, d_units[dunit-BS_DUNIT_B], unit_str_len) == 0) {
pwr = (uint64_t) (dunit - BS_DUNIT_B);
mpfr_pow_ui (dec_mul, dec_mul, pwr, MPFR_RNDN);
mpfr_mul (size, size, dec_mul, MPFR_RNDN);
mpfr_clear (dec_mul);
/* Compute 1000^pwr using repeated multiplication */
mpf_set_ui (dec_mul, 1);
for (i = 0; i < pwr; i++) {
mpf_mul (dec_mul, dec_mul, base);
}
mpf_mul (size, size, dec_mul);
mpf_clear (dec_mul);
mpf_clear (base);
return true;
}

Expand All @@ -256,21 +291,27 @@ static bool multiply_size_by_unit (mpfr_t size, char *unit_str) {
for (bunit=BS_BUNIT_B; bunit < BS_BUNIT_UNDEF; bunit++)
if (strncasecmp (unit_str, _(b_units[bunit-BS_BUNIT_B]), unit_str_len) == 0) {
pwr = (uint64_t) bunit - BS_BUNIT_B;
mpfr_mul_2exp (size, size, 10 * pwr, MPFR_RNDN);
mpf_mul_2exp (size, size, 10 * pwr);
return true;
}

mpfr_init2 (dec_mul, BS_FLOAT_PREC_BITS);
mpfr_set_ui (dec_mul, 1000, MPFR_RNDN);
mpf_set_ui (base, 1000);
for (dunit=BS_DUNIT_B; dunit < BS_DUNIT_UNDEF; dunit++)
if (strncasecmp (unit_str, _(d_units[dunit-BS_DUNIT_B]), unit_str_len) == 0) {
pwr = (uint64_t) (dunit - BS_DUNIT_B);
mpfr_pow_ui (dec_mul, dec_mul, pwr, MPFR_RNDN);
mpfr_mul (size, size, dec_mul, MPFR_RNDN);
mpfr_clear (dec_mul);
/* Compute 1000^pwr using repeated multiplication */
mpf_set_ui (dec_mul, 1);
for (i = 0; i < pwr; i++) {
mpf_mul (dec_mul, dec_mul, base);
}
mpf_mul (size, size, dec_mul);
mpf_clear (dec_mul);
mpf_clear (base);
return true;
}

mpf_clear (dec_mul);
mpf_clear (base);
return false;
}

Expand Down Expand Up @@ -452,7 +493,7 @@ BSSize bs_size_new_from_str (const char *size_str, BSError **error) {
const char *radix_char = NULL;
char *loc_size_str = NULL;
mpf_t parsed_size;
mpfr_t size;
mpf_t size;
int status = 0;
BSSize ret = NULL;
PCRE2_UCHAR *substring = NULL;
Expand Down Expand Up @@ -518,8 +559,7 @@ BSSize bs_size_new_from_str (const char *size_str, BSError **error) {
return NULL;
}

/* parse the number using GMP because it knows how to handle localization
much better than MPFR */
/* parse the number using GMP - it handles localization correctly */
mpf_init2 (parsed_size, BS_FLOAT_PREC_BITS);
status = mpf_set_str (parsed_size, *substring == '+' ? (const char *) substring+1 : (const char *) substring, 10);
pcre2_substring_free (substring);
Expand All @@ -531,9 +571,9 @@ BSSize bs_size_new_from_str (const char *size_str, BSError **error) {
mpf_clear (parsed_size);
return NULL;
}
/* but use MPFR from now on because GMP thinks 0.1*1000 = 99 */
mpfr_init2 (size, BS_FLOAT_PREC_BITS);
mpfr_set_f (size, parsed_size, MPFR_RNDN);
/* Use GMP mpf_t directly - we'll handle rounding correctly when converting to integer */
mpf_init2 (size, BS_FLOAT_PREC_BITS);
mpf_set (size, parsed_size);
mpf_clear (parsed_size);

status = pcre2_substring_get_byname (match_data, (PCRE2_SPTR) "rest", &substring, &substring_len);
Expand All @@ -545,7 +585,7 @@ BSSize bs_size_new_from_str (const char *size_str, BSError **error) {
pcre2_match_data_free (match_data);
pcre2_code_free (regex);
free (loc_size_str);
mpfr_clear (size);
mpf_clear (size);
return NULL;
}
}
Expand All @@ -554,10 +594,12 @@ BSSize bs_size_new_from_str (const char *size_str, BSError **error) {
pcre2_code_free (regex);

ret = bs_size_new ();
mpfr_get_z (ret->bytes, size, MPFR_RNDZ);
/* Convert from mpf_t to mpz_t with round-toward-zero using string conversion
to ensure correct decimal rounding (mpz_set_f truncates binary, giving wrong results) */
mpz_set_f_round_zero (ret->bytes, size);

free (loc_size_str);
mpfr_clear (size);
mpf_clear (size);

return ret;
}
Expand Down
2 changes: 1 addition & 1 deletion src/bytesize.pc.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,4 @@ URL: https://github.com/storaged-project/libbytesize
Version: @VERSION@
Cflags: -I${includedir}/bytesize
Libs: -lbytesize
Libs.private: -lgmp -lmpfr
Libs.private: -lgmp