Created attachment 75749 [details] [review]
The aforementioned patch

Something like this seems more similar to what other frameworks offer:
 g_string_replace(GString *str, int pos, int len, const char *replacement);
 g_string_replace_len(GString *str,
             int pos, int len, const char *replacement, int replacement_len);

OK, replace is possibly a better name for these functions than overwrite. However, I don't get what the first len argument is in your examples... if I say
g_string_replace ("This is a sentence", 5, 8, "isnt"), what happens? Is "isnt" repeated until it fills 8 bytes becoming "This isntisnttence", or are the remaining 4 set to NULLs?

The idea is erase() followed by insert(), except if the erased len is the same as the inserted len, it can optimize by skipping the two memmove() calls.
(Which is what I thought you were getting at with overwrite())

Though, now that I look at Python and Java their replace() does a replace-by-value (replace all occurrences of a given substring), so maybe replace() is a bad name.

splice?

Comment on attachment 75749 [details] [review]
The aforementioned patch

>+GString *
>+g_string_overwrite (GString       *string,
>+		    gssize          pos,

the type for po should be gsize since negative values don't make sense here.

>+		    const gchar   *val)
>+{
>+  gsize len;
>+
>+  g_return_val_if_fail (string != NULL, NULL);
>+  g_return_val_if_fail (val != NULL, string);
>+  g_return_val_if_fail (pos >= 0, string);
>+  g_return_val_if_fail (pos <= string->len, string);
>+
>+  len = strlen (val);
>+
>+  g_string_maybe_expand (string, pos + len - string->len);

correct me if i'm wrong, but using an example calculation
pos=3, len=5, string->len=25:
  3 + 5 - 25 = 8 + (2^32 - 25) = 8 + 4294967271 = 4294967279
(adding 2^32 because len and string->len force unsigned evaluation)
does your test code check for this case btw?

to fix this, one could:
- use MAX (pos + len, string->len) - strig->len
- force signed evaluation by casting all 3 variables and change the
  the internal function g_string_maybe_expand() to accept gssize instead of gsize.

>+
>+  memcpy (string->str + pos, val, len);
>+
>+  if (pos + len > string->len)
>+    string->str[pos + len] = '\0';
>+
>+  string->len = MAX (string->len, pos + len);
>+
>+  return string;
>+}
>+
>+/**
>+ * g_string_overwrite_len:
>+ * @string: a #GString
>+ * @pos: the position at which to start overwriting
>+ * @val: the string that will overwrite the @string starting at @pos
>+ * @len: the number of bytes to write from @val
>+ * 
>+ * Overwrites part of a string, lengthening it if necessary. This function
>+ * will work with embedded NULLs.
>+ * 
>+ * Return value: @string
>+ **/
>+GString *
>+g_string_overwrite_len (GString       *string,
>+			gssize          pos,

use gsize, as above.

>+			const gchar   *val,
>+			gsize          len)

the below code expects a signed len, so you need gssize here.

>+{
>+  g_return_val_if_fail (string != NULL, NULL);
>+  g_return_val_if_fail (val != NULL, string);
>+  g_return_val_if_fail (pos >= 0, string);
>+  g_return_val_if_fail (pos <= string->len, string);
>+
>+  if (len < 0)
>+    len = strlen (val);
>+
>+  g_string_maybe_expand (string, pos + len - string->len);
>+
>+  memcpy (string->str + pos, val, len);
>+
>+  if (pos + len > string->len)
>+    string->str[pos + len] = '\0';
>+
>+  string->len = MAX (string->len, pos + len);
>+
>+  return string;
>+}
>+

the patch in general looks good, but the gsize vs. gssize issues definitely must be fixed.

(In reply to comment #4)
> The idea is erase() followed by insert(), except if the erased len is the same
> as the inserted len, it can optimize by skipping the two memmove() calls.
> (Which is what I thought you were getting at with overwrite())

overwrite makes perfect sense without memove. i don't think the suggested API needs any corrections.

> Though, now that I look at Python and Java their replace() does a
> replace-by-value (replace all occurrences of a given substring), so maybe
> replace() is a bad name.

for the overwriting that this patch implements you're probably right.
g_string_overwrite() is pretty closely naming i'd say, so i'm inclined to take the patch the way it is, provided it's technical issues get fixed.

Created attachment 89982 [details] [review]
Fixed patch

Sorry about how long this took, been caught up in other things. Can this be commited?

Samuel

Comment on attachment 89982 [details] [review]
Fixed patch

>Index: glib/gstring.c
>===================================================================
>--- glib/gstring.c	(revision 5560)
>+++ glib/gstring.c	(working copy)
>@@ -1023,6 +1023,78 @@
> }
> 
> /**
>+ * g_string_overwrite:
>+ * @string: a #GString
>+ * @pos: the position at which to start overwriting
>+ * @val: the string that will overwrite the @string starting at @pos
>+ * 
>+ * Overwrites part of a string, lengthening it if necessary.

this needs: Since: 2.14.

>+ * 
>+ * Return value: @string
>+ **/
>+GString *
>+g_string_overwrite (GString     *string,
>+		    gsize        pos,
>+		    const gchar *val)
>+{
>+  gsize len;
>+
>+  g_return_val_if_fail (string != NULL, NULL);
>+  g_return_val_if_fail (val != NULL, string);
>+  g_return_val_if_fail (pos >= 0 && pos <= string->len, string);
>+
>+  len = strlen (val);
>+
>+  g_string_maybe_expand (string, MAX (pos + len, string->len) - string->len);
>+
>+  memcpy (string->str + pos, val, len);
>+
>+  if (pos + len > string->len)
>+    string->str[pos + len] = '\0';
>+
>+  string->len = MAX (string->len, pos + len);
>+
>+  return string;
>+}
>+
>+/**
>+ * g_string_overwrite_len:
>+ * @string: a #GString
>+ * @pos: the position at which to start overwriting
>+ * @val: the string that will overwrite the @string starting at @pos
>+ * @len: the number of bytes to write from @val
>+ * 
>+ * Overwrites part of a string, lengthening it if necessary. This function
>+ * will work with embedded NULLs.

Since: 2.14

>+ * 
>+ * Return value: @string
>+ **/
>+GString *
>+g_string_overwrite_len (GString       *string,
>+			gsize         pos,
>+			const gchar   *val,
>+			gsize          len)

len needs to be gssize to be negative.

>+{
>+  g_return_val_if_fail (string != NULL, NULL);
>+  g_return_val_if_fail (val != NULL, string);

this is missing if (!len) return string;
so that we allow val==NULL in case len=0.

>+  g_return_val_if_fail (pos >= 0 && pos <= string->len, string);
>+
>+  if (len < 0)
>+    len = strlen (val);

here you allow negative len...

>+
>+  g_string_maybe_expand (string, MAX (pos + len, string->len) - string->len);
>+
>+  memcpy (string->str + pos, val, len);
>+
>+  if (pos + len > string->len)
>+    string->str[pos + len] = '\0';
>+
>+  string->len = MAX (string->len, pos + len);
>+
>+  return string;
>+}
>+
>+/**
>  * g_string_erase:
>  * @string: a #GString
>  * @pos: the position of the content to remove
>Index: glib/gstring.h
>===================================================================
>--- glib/gstring.h	(revision 5560)
>+++ glib/gstring.h	(working copy)
>@@ -105,6 +105,13 @@
> GString*     g_string_insert_unichar    (GString	 *string,
> 					 gssize		  pos,    
> 					 gunichar	  wc);
>+GString*     g_string_overwrite         (GString	 *string,
>+					 gsize		  pos,    
>+					 const gchar	 *val);
>+GString*     g_string_overwrite_len     (GString	 *string,
>+					 gsize		  pos,    
>+					 const gchar	 *val,
>+					 gsize            len);

this needs gssize for len.

> GString*     g_string_erase	        (GString	 *string,
> 					 gssize		  pos,
> 					 gssize		  len);

apart from the above issues, the patch looks good.
tbf, bratsche, can you take care of fixing, testing and committing please?

Created attachment 90005 [details] [review]
Reworked patch.

- Reworked the patch to include Tim's change requests (@Since tag, gssize len, early exit on !len)
- Avoid code duplication by calling g_string_overwrite_len from g_string_overwrite
- Made the unit-tests more pedantic.

rock, thanks, good patch, please test + commit.

2007-06-15  Mathias Hasselmann  <mathias.hasselmann@gmx.de>
       * build, tests/string-test.c, glib/glib.symbols, 
       glib/gstring.c, glib/gstring.h: Introduce g_string_overwrite(_len)?
       for overwriting parts of strings (#368686, Samuel Cormier-Iijima)