After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 57693 - g_string_vprintf ()
g_string_vprintf ()
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: general
unspecified
Other other
: Normal enhancement
: ---
Assigned To: Mathias Hasselmann (IRC: tbf)
gtkdev
: 164446 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2001-07-17 22:02 UTC by Fabrice Bauzac
Modified: 2011-02-18 16:14 UTC
See Also:
GNOME target: ---
GNOME version: Unversioned Enhancement


Attachments
Patch to provide _vprintf() functions, _new_printf(), _clone(), _slice() (6.80 KB, patch)
2005-12-22 02:49 UTC, Paul Evans
none Details | Review
add g_string_(append_)vprintf (4.57 KB, patch)
2007-06-14 19:25 UTC, Allison Karlitskaya (desrt)
committed Details | Review
Restore old behaviour (752 bytes, patch)
2007-06-16 19:42 UTC, Mathias Hasselmann (IRC: tbf)
committed Details | Review
Use memcpy in g_string_append_vprintf (1.05 KB, patch)
2007-06-22 16:30 UTC, Mathias Hasselmann (IRC: tbf)
committed Details | Review

Description Fabrice Bauzac 2001-07-17 22:02:52 UTC
Hello,

There is a g_string_printf () and a g_string_printfa () functions, but
the va_list versions of these functions are not exported (static void
g_string_printfa_internal ()).

I need to do a sort of g_string_vprintfa, but I can't.  So I am
currently doing this:

  char *s;
  s = g_strdup_vprintf (f, v);
  g_string_append (n->outbuf, s);

Wouldn't it be just as easy to let g_string_vprintfa
(g_string_printfa_internal) be an exported function?

TIA.
Comment 1 Owen Taylor 2001-07-18 21:20:01 UTC
Sounds like a reasonable possible future addition, but
since the current implementation of g_string_printfa() is:

static void
g_string_printfa_internal (GString     *string,
			   const gchar *fmt,
			   va_list      args)
{
  gchar *buffer;

  buffer = g_strdup_vprintf (fmt, args);
  g_string_append (string, buffer);
  g_free (buffer);
}

It probably isn't very urgent in terms of efficiency :-)
Comment 2 Paul Evans 2005-12-21 00:27:37 UTC
Is there any change on this bug? It's a feature I'd like to see added. Is there any pressing reason why it shouldn't be added? If not, I'll submit a patch to add it.
Comment 3 Matthias Clasen 2005-12-21 13:11:43 UTC
The va_list versions of these functions are not really that useful, since there is no way to construct a va_list...
Comment 4 Paul Evans 2005-12-22 02:46:38 UTC
Other than, say, va_start()? :)

The point of the va_list functions is that, without them, one cannot make new ...-style functions to wrap them. I wanted to add a g_string_new_printf(). Without any form of vprintf() function in GString, that cannot be done.
Comment 5 Paul Evans 2005-12-22 02:49:32 UTC
Created attachment 56280 [details] [review]
Patch to provide _vprintf() functions, _new_printf(), _clone(), _slice()

This patch contains an implementation of g_string_vprintf() and g_string_append_vprintf(). These just trivially wrap the static g_string_append_printf_internal() function anyway.

Also included (since I'd posted the patch to the mailing list) are some other convenience functions; all of them GString* constructors. g_string_new_printf(), g_string_new_vprintf(), g_string_clone() and g_string_slice().
Comment 6 Dominic Lachowicz 2006-03-27 13:47:45 UTC
*** Bug 164446 has been marked as a duplicate of this bug. ***
Comment 7 Paul "LeoNerd" Evans 2006-03-28 13:42:09 UTC
Is there any progress happening with this patch? I notice that since I submitted it 4 months ago, no reply has been made. Has anyone looked at the patch?

Is it suitable? Is it wanted? I believe the functions provided to be quite useful - I use them a lot in programs I write. If there is some issue with the way they are implemented or anything do let me know. Otherwise, I'd be quite keen to see them merged in to GLib.
Comment 8 Allison Karlitskaya (desrt) 2007-06-14 19:25:40 UTC
Created attachment 89964 [details] [review]
add g_string_(append_)vprintf

       * docs/reference/glib/glib-sections.txt:
       * glib/glib/symbols:
       * glib/gstring.[ch] (g_string_printf_internal): Improve
       performance by removing the use of an intermediate g_malloc'd
       buffer.  Rename to g_string_append_vprintf, document, and expose
       along with g_string_vprintf as new public API.
Comment 9 Mathias Hasselmann (IRC: tbf) 2007-06-15 12:46:30 UTC
Committed with revision 5564. Waiting for more bugzilla power to close this bug.
Comment 10 Andreas Köhler 2007-06-15 22:35:02 UTC
Reopening the bug, because it might have provoked bug 447933.
Any opinions?
Comment 11 Cody Russell 2007-06-15 22:43:42 UTC
Not sure if it's related, closing for now.
Comment 12 Matthias Clasen 2007-06-16 17:25:34 UTC
Mathias, who said to commit this ?

I dislike the "improved" implementation with its valist copying and the use of upper_bound. I very much doubt that doing all the formatting work twice is more efficient than doing one extra malloc. 
Comment 13 Mathias Hasselmann (IRC: tbf) 2007-06-16 19:42:49 UTC
Created attachment 90092 [details] [review]
Restore old behaviour

I commited the patch because that function should have been public for ages. Well, but I have to admit having not benchmarked the patch first. So I changed string-test.c to be suitable for benchmarking and compared the performance of r5563 and r5570 (for i in 1 2 3 4 5; do time ./string-test; done):

--- string-test.c       (Revision 5563)
+++ string-test.c       (Arbeitskopie)
@@ -61,6 +61,8 @@
 main (int   argc,
       char *argv[])
 {
+gint bm;
+for(bm=0; bm<200; ++bm) {
   GStringChunk *string_chunk;
 
   gchar *tmp_string = NULL, *tmp_string_2;
@@ -322,7 +324,7 @@
   g_snprintf (tmp_string, 10, "%2$s %1$s", "a", "b");
   g_assert (strcmp (tmp_string, "b a") == 0);
   g_free (tmp_string);
-
+}
   return 0;
 }

Results for r5570:

user    0m3.644s
user    0m3.640s
user    0m3.620s
user    0m3.620s
user    0m3.624s

Results for r5563:

user    0m3.484s
user    0m3.404s
user    0m3.408s
user    0m3.424s
user    0m3.408s

Seems like _g_gnulib_vasprintf - which is indirectly used by the old variant - does some really good job and outperforms g_printf_string_upper_bound. Double checking with the attached patch, which restores the old _g_gnulib_vasprintf based behaviour confirms that:

user    0m3.488s
user    0m3.416s
user    0m3.404s
user    0m3.432s
user    0m3.416s

So I suggest I commit the attached patch?
Comment 14 Behdad Esfahbod 2007-06-16 19:52:24 UTC
(In reply to comment #13)
> 
> I commited the patch because that function should have been public for ages.

Only the maintainers decide which functions should become public.  Not even regular Gtk+ developers and definitely not other contributors that happen to have commit access.

Please use your commit access responsibly.  You should only commit to branches that you maintain or have been invited to commit to.  For any other project/branch please follow the HACKING file in the module and ask if in doubt.

Thanks.
Comment 15 Mathias Hasselmann (IRC: tbf) 2007-06-18 04:59:52 UTC
Matthias: Excuse me for impatiently crossing the line. Should not happen again. I'll revert the commit if you wish.
Comment 16 Mathias Hasselmann (IRC: tbf) 2007-06-22 16:30:36 UTC
Created attachment 90464 [details] [review]
Use memcpy in g_string_append_vprintf

As suggested by Tim this patches directly uses memcpy.