GNOME Bugzilla – Bug 57693
g_string_vprintf ()
Last modified: 2011-02-18 16:14:11 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.
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 :-)
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.
The va_list versions of these functions are not really that useful, since there is no way to construct a va_list...
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.
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().
*** Bug 164446 has been marked as a duplicate of this bug. ***
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.
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.
Committed with revision 5564. Waiting for more bugzilla power to close this bug.
Reopening the bug, because it might have provoked bug 447933. Any opinions?
Not sure if it's related, closing for now.
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.
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?
(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.
Matthias: Excuse me for impatiently crossing the line. Should not happen again. I'll revert the commit if you wish.
Created attachment 90464 [details] [review] Use memcpy in g_string_append_vprintf As suggested by Tim this patches directly uses memcpy.