Hello,

On Wed, Sep 11, 2002 at 12:23:09AM +0900, Yukihiro Matsumoto wrote:
> Your changes seems OK.  But I want functions to be static as much as
> possible, so rb_str_modify_check() and str_independent() are better to
> be static.  In addition, I miss a few modifies you dropped.  I will do
> it for you.

	I don't want to waste your time with reworking my patches, so I
	made new one. (I'm perfectly OK when you say: not like this, do
	this. I'm definitely more happy when you can work on some more
	important things!)

		Michal

PS: Some comments inside.

Index: string.c
===================================================================
RCS file: /src/ruby/string.c,v
retrieving revision 1.118
diff -u -p -r1.118 string.c
--- string.c	2002/09/11 04:05:36	1.118
+++ string.c	2002/09/11 16:22:13
@@ -27,11 +27,12 @@
 
 VALUE rb_cString;
 
-#define STR_ASSOC   FL_USER3
+#define STR_ASSOC FL_USER3
 
 #define RESIZE_CAPA(str,capacity) do {\
     REALLOC_N(RSTRING(str)->ptr, char, (capacity)+1);\
     RSTRING(str)->aux.capa = (capacity);\
+    FL_UNSET(str, STR_ASSOC);\
 } while (0)
 
 VALUE rb_fs;
@@ -61,18 +62,18 @@ str_new(klass, ptr, len)
     if (len < 0) {
 	rb_raise(rb_eArgError, "negative string size (or size too big)");
     }
-
     str = rb_obj_alloc(klass);
     RSTRING(str)->len = len;
     RSTRING(str)->aux.capa = len;
     RSTRING(str)->ptr = ALLOC_N(char,len+1);
+
     if (ptr) {
 	memcpy(RSTRING(str)->ptr, ptr, len);
+	RSTRING(str)->ptr[len] = '\0';
     }
     else {
-	MEMZERO(RSTRING(str)->ptr, char, len);
+	MEMZERO(RSTRING(str)->ptr, char, len+1);
     }
-    RSTRING(str)->ptr[len] = '\0';
     return str;
 }
 
@@ -212,27 +213,62 @@ rb_str_to_str(str)
     return rb_convert_type(str, T_STRING, "String", "to_str");
 }
 
+static int
+str_check_independent(str)
+    VALUE str;
+{
+    if (OBJ_FROZEN(str)) rb_error_frozen("string");
+    if (!OBJ_TAINTED(str) && rb_safe_level() >= 4) {
+	rb_raise(rb_eSecurityError, "Insecure: can't modify string");
+    }
+    if (!FL_TEST(str, ELTS_SHARED)) return 1;
+    return 0;
+}
+
 static void
+str_make_independent(str)
+    VALUE str;
+{
+    char *ptr;
+    long len = RSTRING(str)->len;
+
+    ptr = ALLOC_N(char, len+1);
+    if (RSTRING(str)->ptr) {
+	memcpy(ptr, RSTRING(str)->ptr, len);
+	ptr[len] = '\0';
+    }
+    else {
+	MEMZERO(ptr, char, len+1);
+    }
+    RSTRING(str)->ptr = ptr;
+    RSTRING(str)->aux.capa = len;
+    FL_UNSET(str, ELTS_SHARED|STR_ASSOC);
+}
+
+void
+rb_str_modify(str)
+    VALUE str;
+{
+    if (!str_check_independent(str)) {
+	str_make_independent(str);
+    }
+}
+
+static void
 rb_str_shared_replace(str, str2)
     VALUE str, str2;
 {
     if (str == str2) return;
-    if (!FL_TEST(str, ELTS_SHARED)) free(RSTRING(str)->ptr);
-    if (NIL_P(str2)) {
-	RSTRING(str)->ptr = 0;
-	RSTRING(str)->len = 0;
-	RSTRING(str)->aux.capa = 0;
-	return;
+    if (str_check_independent(str)) free(RSTRING(str)->ptr);
+
+#if 0
+    if (NIL_P(str2) || FL_TEST(str2, ELTS_SHARED|STR_ASSOC)) {
+	rb_bug("rb_str_shared_replace: Str2 is not PURE String, as matz said!");
     }
+#endif
     RSTRING(str)->ptr = RSTRING(str2)->ptr;
     RSTRING(str)->len = RSTRING(str2)->len;
-    if (FL_TEST(str2, ELTS_SHARED|STR_ASSOC)) {
-	FL_SET(str, RBASIC(str2)->flags & (ELTS_SHARED|STR_ASSOC));
-	RSTRING(str)->aux.shared = RSTRING(str2)->aux.shared;
-    }
-    else {
-	RSTRING(str)->aux.capa = RSTRING(str2)->aux.capa;
-    }
+    RSTRING(str)->aux.capa = RSTRING(str2)->aux.capa;
     RSTRING(str2)->ptr = 0;	/* abandon str2 */
     RSTRING(str2)->len = 0;
     RSTRING(str2)->aux.capa = 0;
@@ -250,16 +286,12 @@ rb_str_associate(str, add)
     }
     else {
 	if (FL_TEST(str, ELTS_SHARED)) {
-	    rb_str_modify(str);
+	    str_make_independent(str);
 	}
-	else if (RSTRING(str)->aux.shared) {
-	    /* str_buf */
-	    if (RSTRING(str)->aux.capa != RSTRING(str)->len) {
-		RESIZE_CAPA(str, RSTRING(str)->len);
-	    }
+	if (RSTRING(str)->aux.capa != RSTRING(str)->len) {
+	    RESIZE_CAPA(str, RSTRING(str)->len);
 	}
 	RSTRING(str)->aux.shared = add;
-	FL_UNSET(str, ELTS_SHARED);
 	FL_SET(str, STR_ASSOC);
     }
 }
@@ -398,41 +430,6 @@ rb_str_format(str, arg)
     return rb_f_sprintf(2, argv);
 }
 
-static int
-str_independent(str)
-    VALUE str;
-{
-    if (OBJ_FROZEN(str)) rb_error_frozen("string");
-    if (!OBJ_TAINTED(str) && rb_safe_level() >= 4)
-	rb_raise(rb_eSecurityError, "Insecure: can't modify string");
-    if (!FL_TEST(str, ELTS_SHARED)) return 1;
-    return 0;
-}
-
-static void
-str_make_independent(str)
-    VALUE str;
-{
-    char *ptr;
-
-    ptr = ALLOC_N(char, RSTRING(str)->len+1);
-    if (RSTRING(str)->ptr) {
-	memcpy(ptr, RSTRING(str)->ptr, RSTRING(str)->len);
-    }
-    ptr[RSTRING(str)->len] = 0;
-    RSTRING(str)->ptr = ptr;
-    RSTRING(str)->aux.capa = RSTRING(str)->len;
-    FL_UNSET(str, ELTS_SHARED|STR_ASSOC);
-}
-
-void
-rb_str_modify(str)
-    VALUE str;
-{
-    if (!str_independent(str))
-	str_make_independent(str);
-}
-
 VALUE
 rb_string_value(ptr)
     volatile VALUE *ptr;
@@ -493,10 +490,18 @@ VALUE
 rb_str_dup_frozen(str)
     VALUE str;
 {
+#if 0
+    /*
+     * Question:
+     * What is the str is subseq of share
+     * (str->len < str->aux.shared->len)
+     * ?
+     */
     if (FL_TEST(str, ELTS_SHARED)) {
 	OBJ_FREEZE(RSTRING(str)->aux.shared);
 	return RSTRING(str)->aux.shared;
     }
+#endif
     if (OBJ_FROZEN(str)) return str;
     str = rb_str_dup(str);
     OBJ_FREEZE(str);
@@ -511,15 +516,14 @@ rb_str_resize(str, len)
     if (len < 0) {
 	rb_raise(rb_eArgError, "negative string size (or size too big)");
     }
-	
     if (len != RSTRING(str)->len) {
 	rb_str_modify(str);
 
 	if (RSTRING(str)->len < len || RSTRING(str)->len - len > 1024) {
 	    RESIZE_CAPA(str, len);
 	}
-	RSTRING(str)->len = len;
 	RSTRING(str)->ptr[len] = '\0';	/* sentinel */
+	RSTRING(str)->len = len;
     }
     return str;
 }
@@ -532,10 +536,20 @@ rb_str_buf_cat(str, ptr, len)
 {
     long capa, total;
 
+    if (len < 0) {
+	rb_raise(rb_eArgError, "negative string size (or size too big)");
+    }
+    if (len == 0) return str;
     if (FL_TEST(str, ELTS_SHARED)) {
-	rb_str_modify(str);
+	str_make_independent(str);
+    }
+    if (FL_TEST(str, STR_ASSOC)) {
+	FL_UNSET(str, STR_ASSOC);
+	capa = RSTRING(str)->len;
     }
-    capa = RSTRING(str)->aux.capa;
+    else {
+	capa = RSTRING(str)->aux.capa;
+    }
     total = RSTRING(str)->len+len;
     if (capa <= total) {
 	while (total > capa) {
@@ -544,8 +558,8 @@ rb_str_buf_cat(str, ptr, len)
 	RESIZE_CAPA(str, capa);
     }
     memcpy(RSTRING(str)->ptr + RSTRING(str)->len, ptr, len);
-    RSTRING(str)->len = total;
     RSTRING(str)->ptr[total] = '\0'; /* sentinel */
+    RSTRING(str)->len = total;
 
     return str;
 }
@@ -565,22 +579,7 @@ rb_str_cat(str, ptr, len)
     long len;
 {
     rb_str_modify(str);
-    if (len > 0) {
-	if (!FL_TEST(str, ELTS_SHARED) && !FL_TEST(str, STR_ASSOC)) {
-	    return rb_str_buf_cat(str, ptr, len);
-	}
-	RESIZE_CAPA(str, RSTRING(str)->len + len);
-	if (ptr) {
-	    memcpy(RSTRING(str)->ptr + RSTRING(str)->len, ptr, len);
-	}
-	else {
-	    MEMZERO(RSTRING(str)->ptr + RSTRING(str)->len, char, len);
-	}
-	RSTRING(str)->len += len;
-	RSTRING(str)->ptr[RSTRING(str)->len] = '\0'; /* sentinel */
-    }
-
-    return str;
+    return rb_str_buf_cat(str, ptr, len);
 }
 
 VALUE
@@ -598,11 +597,17 @@ rb_str_buf_append(str, str2)
     long capa, len;
 
     if (FL_TEST(str, ELTS_SHARED)) {
-	rb_str_modify(str);
+	str_make_independent(str);
     }
-    capa = RSTRING(str)->aux.capa;
-
+    if (FL_TEST(str, STR_ASSOC)) {
+	FL_UNSET(str, STR_ASSOC);
+	capa = RSTRING(str)->len;
+    }
+    else {
+	capa = RSTRING(str)->aux.capa;
+    }
     len = RSTRING(str)->len+RSTRING(str2)->len;
+    
     if (capa <= len) {
 	while (len > capa) {
 	    capa = (capa + 1) * 2;
@@ -621,25 +626,12 @@ VALUE
 rb_str_append(str, str2)
     VALUE str, str2;
 {
-    long len;
-
-    StringValue(str2);
     rb_str_modify(str);
-    if (RSTRING(str2)->len > 0) {
-	len = RSTRING(str)->len+RSTRING(str2)->len;
-	if (!FL_TEST(str, ELTS_SHARED) && !FL_TEST(str, STR_ASSOC)) {
-	    rb_str_buf_append(str, str2);
-	    OBJ_INFECT(str, str2);
-	    return str;
-	}
-	RESIZE_CAPA(str, len);
-	memcpy(RSTRING(str)->ptr + RSTRING(str)->len,
-	       RSTRING(str2)->ptr, RSTRING(str2)->len);
-	RSTRING(str)->len += RSTRING(str2)->len;
-	RSTRING(str)->ptr[RSTRING(str)->len] = '\0'; /* sentinel */
-    }
-    OBJ_INFECT(str, str2);
+    StringValue(str2);
 
+    rb_str_buf_append(str, str2);
+
+    OBJ_INFECT(str, str2);
     return str;
 }
 
@@ -1575,7 +1567,7 @@ str_gsub(argc, argv, str, bang)
     }
     rb_backref_set(match);
     if (bang) {
-	if (str_independent(str)) {
+	if (!FL_TEST(str, ELTS_SHARED)) {
 	    free(RSTRING(str)->ptr);
 	}
 	FL_UNSET(str, ELTS_SHARED|STR_ASSOC);
@@ -1601,6 +1593,7 @@ rb_str_gsub_bang(argc, argv, str)
     VALUE *argv;
     VALUE str;
 {
+    str_check_independent(str);
     return str_gsub(argc, argv, str, 1);
 }
 
@@ -1619,23 +1612,27 @@ rb_str_replace(str, str2)
 {
     if (str == str2) return str;
 
+    str_check_independent(str);
     StringValue(str2);
+
     if (FL_TEST(str2, ELTS_SHARED)) {
-	if (str_independent(str)) {
+	if (!FL_TEST(str, ELTS_SHARED)) {
 	    free(RSTRING(str)->ptr);
 	}
 	RSTRING(str)->len = RSTRING(str2)->len;
 	RSTRING(str)->ptr = RSTRING(str2)->ptr;
-	FL_SET(str, RBASIC(str2)->flags & (ELTS_SHARED|STR_ASSOC));
 	RSTRING(str)->aux.shared = RSTRING(str2)->aux.shared;
+	FL_SET(str, ELTS_SHARED);
     }
     else {
-	rb_str_modify(str);
+	if (FL_TEST(str, ELTS_SHARED)) {
+	    str_make_independent(str);
+	}
 	rb_str_resize(str, RSTRING(str2)->len);
 	memcpy(RSTRING(str)->ptr, RSTRING(str2)->ptr, RSTRING(str2)->len);
 	if (FL_TEST(str2, STR_ASSOC)) {
-	    FL_SET(str, RBASIC(str2)->flags & (ELTS_SHARED|STR_ASSOC));
 	    RSTRING(str)->aux.shared = RSTRING(str2)->aux.shared;
+	    FL_SET(str, STR_ASSOC);
 	}
     }
 
@@ -2295,6 +2292,7 @@ rb_str_delete_bang(argc, argv, str)
     if (argc < 1) {
 	rb_raise(rb_eArgError, "wrong number of arguments");
     }
+    rb_str_modify(str);
     for (i=0; i<argc; i++) {
 	VALUE s = argv[i];
 
@@ -2302,8 +2300,6 @@ rb_str_delete_bang(argc, argv, str)
 	tr_setup_table(s, squeez, init);
 	init = 0;
     }
-
-    rb_str_modify(str);
     s = t = RSTRING(str)->ptr;
     if (!s || RSTRING(str)->len == 0) return Qnil;
     send = s + RSTRING(str)->len;
@@ -2344,6 +2340,7 @@ rb_str_squeeze_bang(argc, argv, str)
     int init = 1;
     int i;
 
+    rb_str_modify(str);
     if (argc == 0) {
 	for (i=0; i<256; i++) {
 	    squeez[i] = 1;
@@ -2358,8 +2355,6 @@ rb_str_squeeze_bang(argc, argv, str)
 	    init = 0;
 	}
     }
-
-    rb_str_modify(str);
     s = t = RSTRING(str)->ptr;
     if (!s || RSTRING(str)->len == 0) return Qnil;
     send = s + RSTRING(str)->len;
-- 
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Michal Rokos                         Czech Technical University, Prague
e-mail: m.rokos / sh.cvut.cz    icq: 36118339     jabber: majkl / jabber.cz
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-