GNOME Bugzilla – Bug 106973
g_string_new usage
Last modified: 2011-02-18 15:57:04 UTC
Use g_string_new (NULL) and save a strlen and a g_string_append. glib/glib/gmarkup.c: str = g_string_new (""); glib/glib/gmarkup.c: context->partial_chunk = g_string_new (""); glib/glib/gmarkup.c: str = g_string_new (""); glib/glib/gmessages.c: gstring = g_string_new (""); glib/glib/gscanner.c: gstring = g_string_new (""); glib/glib/gscanner.c: gstring = g_string_new (""); glib/glib/gscanner.c: gstring = g_string_new (""); glib/glib/gscanner.c: gstring = g_string_new (""); glib/glib/gscanner.c: gstring = g_string_new (""); glib/glib/gshell.c: retval = g_string_new (""); glib/glib/gshell.c: *token = g_string_new (""); glib/glib/gspawn-win32-helper.c: debugstring = g_string_new (""); glib/glib/gspawn-win32-helper.c: debugstring = g_string_new (""); glib/glib/gspawn-win32-helper.c: debugstring = g_string_new (""); glib/glib/gspawn-win32-helper.c: debugstring = g_string_new (""); glib/glib/gspawn-win32.c: outstr = g_string_new (""); glib/glib/gspawn-win32.c: errstr = g_string_new (""); glib/glib/gspawn.c: outstr = g_string_new (""); glib/glib/gspawn.c: errstr = g_string_new (""); glib/gobject/gvaluetransform.c: GString *gstring = g_string_new (""); glib/tests/string-test.c: string2 = g_string_new (""); glib/tests/testglib.c: string2 = g_string_new ("");
if this is an issue why not save apps having to think about it with a check like "if (s == NULL || *s == '\0')" in g_string_new()?
That's silly. Just run the perl one-liner to fix them all instead of slowing down the common path. This showed up because gnumeric was spending too much time in strlen, mostly because of this kind of silliness. Millions of calls add up over time.
Is it a one-time perl script or will app authors continue to do this? Seems likely they'll habitually do it. Doesn't seem like "*s == '\0'" is likely to show up in profiles, and if you pass in NULL for s you don't even hit the *s == '\0', you only hit it if you were going to run strlen anyway
Application authors can write slow code any which way they please. I am more concerned by libraries. Yes, it'll have to be revisited from time to time with no great haste. (After all, the "" versions is functionally fine.) That's not much different from * memory and file descriptor leaks * incorrect isdigit/tolower/... usage * printf ("...%d...", ..., getpid (), ...) * oink = *((guint32*)(random_ptr)) * char *oink = "text"; and other systematic mistakes people get themselves into all the time. (And while I am at it, why does this function end up calling strlen *twice*? It should really cache the length and use g_string_append_len.)
Created attachment 14621 [details] [review] patch
Here is a proposal for a g_string_new which goes straight to g_string_sized_new for NULL or "". The chapest replacement for g_string_new ("") is probably g_string_sized_new (0).
Created attachment 14637 [details] [review] s/g_string_new ("")/g_string_new (NULL)/
That patch came from this script: perl -pi -e 's/g_string_new \(""\)/g_string_new (NULL)/g' */*.c
The patch to g_string_new() looks good to me. I'd go ahead and commit it to both glib-2-2 and HEAD. The patch to change g_string_new("") to g_string_new(NULL) looks fine to me too ... while I very much doubt it will be timable, there is some overhead to a string constant, even if it is "". (Actually, I'd probably omit the changes to the two test cases, it makes them less clear for no benefit.)
Committed to both branches, omitting the test changes.