What’s the use case?

(In reply to Philip Withnall from comment #1)
> What’s the use case?

Formatting transfer (network/disk) speed for use in System Monitor and Gnome Usage.

So you’d typically expect it to return formatted results in Mb and Gb, rather than just bits?

If so, seems reasonable. Patch welcome. :-)

Hello, in fact, to cover our usage, we will also need an additional feature. In the graph UI, we display a scale, and for consistency, it's important to use the same unit for every graduation. So our version has a third argument (a maximum value) that is used to find the unit to format the value.

Would it be possible to emulate this with additional flags like FORCE_KILO, FORCE_MEGA, etc ?

(In reply to Benoît Dejean from comment #5)
> Would it be possible to emulate this with additional flags like FORCE_KILO,
> FORCE_MEGA, etc ?

I’m not so happy about that, since that’s a lot less general purpose. It’s starting to sound like you should implement your own version of g_format_size_full() with the functionality you need. It’s not a complex function, and you can copy the implementation from GLib if the license of GNOME Usage is compatible.

We had one long before you :-)
OK, let's stick to the bits flag only.
I'll make the patch.

Created attachment 361901 [details] [review]
G_FORMAT_SIZE_BITS

Not sure how you want to name it. It passes basic tests.
I don't know what's the policy about errors: it makes no sense to ask for bits & IEC, so I've added a simple assert.

Review of attachment 361901 [details] [review]:

::: glib/gutils.c
@@ +2172,3 @@
  *     a strong "power of 2" basis, like RAM sizes or RAID stripe sizes.
  *     Network and storage sizes should be reported in the normal SI units.
+ * @G_FORMAT_SIZE_BITS: use bits units. Cannot be used with

I’d rephrase this as:

@G_FORMAT_SIZE_BITS: Treat the size as a quantity in bits, rather than bytes, and return units in bits. For example, ‘Mb’ rather than ‘MB’.

@@ +2202,3 @@
   GString *string;
 
+  g_assert(!(flags & (G_FORMAT_SIZE_IEC_UNITS | G_FORMAT_SIZE_BITS)));

Why does it have to be mutually exclusive with G_FORMAT_SIZE_IEC_UNITS?

Mib should be just as valid a unit as MiB, Mb or MB.

Aside: This should be g_return_val_if_fail() rather than g_assert(), as it bails out gracefully.

::: glib/gutils.h
@@ +183,3 @@
   G_FORMAT_SIZE_LONG_FORMAT = 1 << 0,
+  G_FORMAT_SIZE_IEC_UNITS   = 1 << 1,
+  G_FORMAT_SIZE_BITS        = 2 << 1

Please add a trailing comma to the new line to prevent diff noise in future if another element is added after it.

(In reply to Philip Withnall from comment #9)

> ::: glib/gutils.h
> @@ +183,3 @@
>    G_FORMAT_SIZE_LONG_FORMAT = 1 << 0,
> +  G_FORMAT_SIZE_IEC_UNITS   = 1 << 1,
> +  G_FORMAT_SIZE_BITS        = 2 << 1
> 
> Please add a trailing comma to the new line to prevent diff noise in future
> if another element is added after it.

No, don't do this, otherwise C++ developers will be very angry at us, and file bugs to remove the trailing comma.

The support for trailing commas in enumerations was added in C99, but C++03 was forked off from C89, and no new C++ standard decided to fix this.

Ok for the rephrasing.

(In reply to Philip Withnall from comment #9)
> Review of attachment 361901 [details] [review] [review]:
> 
> ::: glib/gutils.c
> @@ +2172,3 @@
>   *     a strong "power of 2" basis, like RAM sizes or RAID stripe sizes.
>   *     Network and storage sizes should be reported in the normal SI units.
> + * @G_FORMAT_SIZE_BITS: use bits units. Cannot be used with
> 
> I’d rephrase this as:
> 
> @G_FORMAT_SIZE_BITS: Treat the size as a quantity in bits, rather than
> bytes, and return units in bits. For example, ‘Mb’ rather than ‘MB’.

OK


> @@ +2202,3 @@
>    GString *string;
>  
> +  g_assert(!(flags & (G_FORMAT_SIZE_IEC_UNITS | G_FORMAT_SIZE_BITS)));
> 
> Why does it have to be mutually exclusive with G_FORMAT_SIZE_IEC_UNITS?
> 
> Mib should be just as valid a unit as MiB, Mb or MB.

I've never heard anyone using mebibits, but I'll add it.


> Aside: This should be g_return_val_if_fail() rather than g_assert(), as it
> bails out gracefully.
> 
> ::: glib/gutils.h
> @@ +183,3 @@
>    G_FORMAT_SIZE_LONG_FORMAT = 1 << 0,
> +  G_FORMAT_SIZE_IEC_UNITS   = 1 << 1,
> +  G_FORMAT_SIZE_BITS        = 2 << 1
> 
> Please add a trailing comma to the new line to prevent diff noise in future
> if another element is added after it.

That's actually "1 << 2" and I agree with Emmanuele that we should not have a trailing comma.

Created attachment 361966 [details] [review]
refactor g_format_size_full

Before submitting the patch to add kb and Kib, would you accept this refactoring that would make the addition of 2 new units easier (a lot less of copy/paste code).

   text    data     bss     dec     hex filename
   8622       0     168    8790    2256 ./glib/.libs/libglib_2_0_la-gutils.o
   8304       0     168    8472    2118 ./glib/.libs/libglib_2_0_la-gutils.o (patched)

Review of attachment 361966 [details] [review]:

This looks like a good cleanup, thanks. I have a few nitpicks, but other than these it looks good.

::: glib/gutils.c
@@ +2243,2 @@
+  GString *string;
+  int index = -1;

Add a typedef to the enum above and declare this to be of that type.

@@ +2261,3 @@
+      const struct FormatPlural * const f = &format_plurals[index];
+      g_string_printf (string,
+                       g_dngettext(GETTEXT_PACKAGE,

Nitpick: Missing space before `(`

@@ +2270,3 @@
+  else
+    {
+      const size_t n = G_N_ELEMENTS(formats[index]);

Nitpick: Missing space before `(`

@@ +2281,1 @@
+      for (size_t i = 1; i < G_N_ELEMENTS(formats[index]); i++)

Can’t use inline declarations.

Missing a space before the `(` in `G_N_ELEMENTS`.

Noted.

But what version of  C standard does glib stick to ?
It doesn't compile at all with c89, and with gnu89 it actually fails in g_format_size_full:

in gutils.c:2337:43: error: ISO C ne supporte pas le fanion « ' » de printf [-Werror=format=]
       formatted_number = g_strdup_printf ("%'"G_GUINT64_FORMAT, size);

I would assume not C99 because you told me not to mixed code and declarations.
Or is it only a matter of style ?

There’s a handy wiki page for it, which I was reminded of today:

https://wiki.gnome.org/Projects/GLib/CompilerRequirements

Created attachment 362256 [details] [review]
0001-utils-refactor-g_format_size_full.patch

Review of attachment 362256 [details] [review]:

::: glib/gutils.c
@@ +2212,1 @@
+  enum FormatIndex

Nitpick: Use

typedef enum
{

} FormatIndex;

since we don’t use the ‘enum’ prefix in GLib.

@@ +2244,2 @@
+  GString *string;
+  enum FormatIndex index = FORMAT_UNDEFINED;

No need for `FORMAT_UNDEFINED`, since this is unconditionally initialised just below. In fact, it’s less safe to unconditionally initialise this, as it means the compiler (or analysis tools) won’t give you a used-before-initialised warning if the code below changes, and we don’t ever want to dereference things with FORMAT_UNDEFINED.

@@ +2272,3 @@
+    {
+      const size_t n = G_N_ELEMENTS (formats[index]);
+      size_t i;

Nitpick: For consistency within GLib, please use gsize instead of size_t (they’re equivalent).

@@ +2278,3 @@
+        and then then scan all formats, starting with the 2nd one
+        because the 1st is already managed by with the plural form
+      */

Nitpick: Please prefix each line in a multi-line comment with asterisks.

/*
 * i.e. Write it
 * like this.
 */

@@ +2293,3 @@
     }
 
+

Spurious additional new line.

Created attachment 362274 [details] [review]
0001-utils-refactor-g_format_size_full.patch

Review of attachment 362274 [details] [review]:

Great, thanks.

51f9c95cf240b7de4c1db8e4dcb7e18d72ba0d3c made this appear in .po files:

#: ../glib/gutils.c:2220 ../glib/gutils.c:2221
#, c-format
msgid "%u bytes"
msgstr ""

…which is very wrong for i18n.

https://wiki.gnome.org/TranslationProject/DevGuidelines/Plurals

I understand but in the end, it gets called with g_dngettext.
Maybe we should a comment for translators before lines 2220 and 2221 ?

(I'm actually more concerned about the string butchering happening starting at https://git.gnome.org/browse/glib/tree/glib/gutils.c#n2328 to make the long format output)

(In reply to Benoît Dejean from comment #21)
> I understand but in the end, it gets called with g_dngettext.
> Maybe we should a comment for translators before lines 2220 and 2221 ?
> 

I don’t understand why you are asking translators to translate a string that won’t be used, and also can’t be translated correctly in most languages.

Those strings are defined there:

  struct FormatPlural format_plurals[2] =
    {
      { N_("%u byte"), N_("%u bytes") },
      { N_("%u byte"), N_("%u bytes") }
    };

and later they are really used:

      g_string_printf (string,
                       g_dngettext (GETTEXT_PACKAGE,
                                    _(f->singular),
                                    _(f->plural),
                                    (guint) size),
                       (guint) size);

#: ../glib/gutils.c:2220 ../glib/gutils.c:2221 ../glib/gutils.c:2377
#, c-format
msgid "%u byte"
msgid_plural "%u bytes"
msgstr[0] ""
msgstr[1] ""

#: ../glib/gutils.c:2220 ../glib/gutils.c:2221
#, c-format
msgid "%u bytes"
msgstr ""


This looks all kinds of wrong. :(

I’ve reverted the patch until a fix is found, so we don’t end up with unnecessary translation churn. (Revert is commit f43babfea.)

(In reply to Benoît Dejean from comment #23)
> Those strings are defined there:
> 
>   struct FormatPlural format_plurals[2] =
>     {
>       { N_("%u byte"), N_("%u bytes") },
>       { N_("%u byte"), N_("%u bytes") }
>     };
> 
> and later they are really used:
> 
>       g_string_printf (string,
>                        g_dngettext (GETTEXT_PACKAGE,
>                                     _(f->singular),
>                                     _(f->plural),
>                                     (guint) size),
>                        (guint) size);

Given that the FORMAT_BYTES and FORMAT_BYTES_IEC entries in format_plurals are identical, could we not drop the format_plurals array and put the strings directly into the g_dngettext() call? I suspect that should fix the string extraction and POT file generation.

(In reply to Emmanuele Bassi (:ebassi) from comment #10)
> (In reply to Philip Withnall from comment #9)
> > Please add a trailing comma to the new line to prevent diff noise in future
> > if another element is added after it.
> 
> No, don't do this, otherwise C++ developers will be very angry at us, and
> file bugs to remove the trailing comma.
> 
> The support for trailing commas in enumerations was added in C99, but C++03
> was forked off from C89, and no new C++ standard decided to fix this.

C++11 did: https://stackoverflow.com/questions/6372650/trailing-commas-and-c

(In reply to Philip Withnall from comment #26)
> Given that the FORMAT_BYTES and FORMAT_BYTES_IEC entries in format_plurals
> are identical, could we not drop the format_plurals array and put the
> strings directly into the g_dngettext() call? I suspect that should fix the
> string extraction and POT file generation.

That table will be extended for bits.
But if you want, we can replace it with a if/else that will only duplicate one call.

(In reply to Benoît Dejean from comment #28)
> (In reply to Philip Withnall from comment #26)
> > Given that the FORMAT_BYTES and FORMAT_BYTES_IEC entries in format_plurals
> > are identical, could we not drop the format_plurals array and put the
> > strings directly into the g_dngettext() call? I suspect that should fix the
> > string extraction and POT file generation.
> 
> That table will be extended for bits.
> But if you want, we can replace it with a if/else that will only duplicate
> one call.

Unless you’ve got any better suggestions, that would be the approach I’d try. I suspect the problem is that xgettext looks for g_dngettext() calls and extracts plural strings from them; and doesn’t know how to extract plural strings from anything else (specifically, not N_() macros).

Created attachment 362332 [details] [review]
0001-utils-refactor-g_format_size_full.patch

Here's a version that keeps the plain call to g_dngettext.
I've checked the statistics for fr.po before and after this patch and they are the same.

Created attachment 362333 [details] [review]
0001-utils-refactor-g_format_size_full.patch

And there was an off-by-one error as "%.1f KiB" requires 9 bytes of storage.

Created attachment 362364 [details] [review]
0001-utils-refactor-g_format_size_full

More const.

Review of attachment 362364 [details] [review]:

++

Is there anything else to fix here, I see that the fix is already on master since a couple of days. Can this be marked as fixed?

Created attachment 362811 [details] [review]
0001-utils-new-G_FORMAT_SIZE_BITS-flag-to-g_format_size_f

Review of attachment 362811 [details] [review]:

A couple of nitpicks, but this looks good. I’ll fix the nitpicks when I push. Thanks.

::: glib/gutils.c
@@ +2275,3 @@
+      break;
+    case (G_FORMAT_SIZE_BITS | G_FORMAT_SIZE_IEC_UNITS):
+      index= FORMAT_BITS_IEC;

Nitpick: Missing a space before `=`.

@@ +2278,3 @@
+      break;
+    default:
+      g_assert_not_reached();

Nitpick: Please add a space before the `(`.

I also tweaked the commit message to be the right length before I pushed.