Re: glib strings (fwd)

[Owen Taylor <otaylor redhat com>]

Dan Libby <danda@epinions-inc.com>:

> Patches for GString attached (from 1.2.8).  Apologies in advance for any
> spacing weirdness. I don't normally use tabs.

You don't need to use tabs - spaces are fine, as long as 8 spaces
corresponds to a tab. But following GTK+ indentation style is 
something we require for patches.

> diff -u -r 1.2.8.orig/glib-1.2.8/glib.h glib-1.2.8/glib.h
> --- 1.2.8.orig/glib-1.2.8/glib.h	Thu Mar 23 18:34:01 2000
> +++ glib-1.2.8/glib.h	Sun Aug 13 19:01:56 2000
> @@ -1612,6 +1612,11 @@
>  void	      g_string_chunk_free	   (GStringChunk *chunk);
>  gchar*	      g_string_chunk_insert	   (GStringChunk *chunk,
>  					    const gchar	 *string);
> +gchar*	      g_string_chunk_insert_n	   (GStringChunk *chunk,
> +					    const gchar	 *string,
> +					    gint	 len);

I don't really know enough about GStringChunk to evaluate the
usefulness of this. I've never actually had an application for
GStringChunk. But the correct naming would be:

 g_string_chunk_insert_len().  

> +gchar*	      g_string_chunk_insert_g	   (GStringChunk *chunk,
> +					    const GString *string);

I don't think this variant is useful. As long as you have strings
without embedded NULs, you can just use:

 g_string_chunk_insert (chunk, string->str);

And I don't think GStringChunk is meant to work for strings with
embedded NULs. (g_string_chunk_insert_const() can't at a fairly
fundemental level, and it would be confusing if g_string_chunk_insert()
behaved differently.)

> @@ -1619,24 +1624,49 @@
>  /* Strings
>   */
>  GString* g_string_new	    (const gchar *init);
> +GString* g_string_new_n	    (const gchar *init,
> +			     gint	  len);

Note that we have now in GLib-1.3.x:

 GString*
 g_string_insert_len (GString     *fstring,
		     gint         pos,
		     const gchar *val,
		     gint         len)
 g_string_append_len()
 g_string_prepend_len()

So that is the approved naming. I'm a little hesitant to
add new_len() and assign_len() variants - simply because
it gets pretty cumbersome to have a huge API, and you can
achieve the same thing, with only a tiny bit more inefficiency. 
But it could be done.

> +GString* g_string_new_g	    (const GString *init);

Two separate issues: 

 - Despite the presence of insert_c(), cryptic one character
   namings are discouraged in GLib/GTK+. The name would
   need to be something like:

    g_string_insert_string ()

 - I don't actually think that these variants are that 
   useful.
 
    g_string_insert_len (string, str->str, str->len);

   Isn't that much more paintful to type. But more importantly, I
   think inserting (etc.) a slice of one string in another string is
   more common operation than inserting a whole string. And
   when you are doing that, you need to fall back to the
   g_string_insert() variant.

   If you did want to add this variant, I'd probably say -
   just add it for g_string_insert(), and not everywhere.

   append == g_string_insert (str, -1, ...)
   prepend == g_string_insert (str, 0, ...)

My general thinking here is that you have to be a little
careful with GString - it is useful tool around, but you
can't use it like C++ string - trying to replace char *
with it would lead to enormous inefficiency.

(Actually, trying to replace char * with string in C++ leads
to enormous inefficiency ... just look at the generated
code size some time.)

Regards,
                                        Owen