From: Yukihiro Matsumoto <matz@...>
Date: 2009-06-02T07:55:11+09:00
Subject: [ruby-core:23664] Re: [Bug #1550] String#lstrip! raises RuntimeError on Frozen String Despite Making No Changes

Hi,

In message "Re: [ruby-core:23663] Re: [Bug #1550] String#lstrip! raises RuntimeError on Frozen String Despite Making No Changes"
    on Tue, 2 Jun 2009 07:14:11 +0900, James Gray <james@grayproductions.net> writes:

|> I admit the inconsistency.  There could be two policies:
|>
|> (a) methods should raise RuntimeError only when actual changes are
|>     made, because it should detect changes.
|>
|> (b) methods should eagerly raise RuntimeError when they could
|>     possibly make changes, to detect modify attempt to frozen
|>     strings (that most likely caused by bugs) earlier.
|>
|> Currently both policies mixed.  You've suggested the former, I'd
|> rather prefer the latter.  Any opinion?
|
|I really feel (b) is the better choice.  The fact that it worked is  
|pretty irrelevant.  It shouldn't have worked.  Either you didn't need  
|to call something like strip!() and you can remove it, or you  
|sometimes do need to call it and you'll need to change the code not to  
|work on frozen data.  Either way, you need to change your code and the  
|error shows that.

I have investigated the source and the following methods use the
policy (a):

  sub/gsub
  rstrip
  chop
  chomp

Other bang methods use the policy (b).  I fixed the local copy.

							matz.

diff --git a/ChangeLog b/ChangeLog
index 4d0a55e..3d6da7b 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+Tue Jun  2 07:44:43 2009  Yukihiro Matsumoto  <matz@ruby-lang.org>
+
+	* string.c (rb_str_gsub_bang): modify check at the beginning.
+	  [ruby-core:23662]  ref [ruby-core:23657]
+
+	* string.c (rb_str_rstrip_bang): ditto.  [ruby-core:23657]
+
+	* string.c (rb_str_chop_bang): ditto.
+
+	* string.c (rb_str_chomp_bang): ditto.
+
 Mon Jun  1 11:21:29 2009  Nobuyoshi Nakada  <nobu@ruby-lang.org>
 
 	* cont.c (cont_capture, fiber_store): reraise transferred error.
diff --git a/string.c b/string.c
index ef25fa5..74ee2a5 100644
--- a/string.c
+++ b/string.c
@@ -3744,7 +3744,6 @@ str_gsub(int argc, VALUE *argv, VALUE str, int bang)
                 val = rb_obj_as_string(val);
             }
 	    str_mod_check(str, sp, slen);
-	    if (bang) str_frozen_check(str);
 	    if (val == dest) { 	/* paranoid check [ruby-dev:24827] */
 		rb_raise(rb_eRuntimeError, "block should not cheat");
 	    }
@@ -3808,6 +3807,7 @@ str_gsub(int argc, VALUE *argv, VALUE str, int bang)
 static VALUE
 rb_str_gsub_bang(int argc, VALUE *argv, VALUE str)
 {
+    str_modify_keep_cr(str);
     return str_gsub(argc, argv, str, 1);
 }
 
@@ -6044,9 +6044,9 @@ chopped_length(VALUE str)
 static VALUE
 rb_str_chop_bang(VALUE str)
 {
+    str_modify_keep_cr(str);
     if (RSTRING_LEN(str) > 0) {
 	long len;
-	rb_str_modify(str);
 	len = chopped_length(str);
 	STR_SET_LEN(str, len);
 	RSTRING_PTR(str)[len] = '\0';
@@ -6100,6 +6100,7 @@ rb_str_chomp_bang(int argc, VALUE *argv, VALUE str)
     char *p, *pp, *e;
     long len, rslen;
 
+    str_modify_keep_cr(str);
     len = RSTRING_LEN(str);
     if (len == 0) return Qnil;
     p = RSTRING_PTR(str);
@@ -6108,7 +6109,6 @@ rb_str_chomp_bang(int argc, VALUE *argv, VALUE str)
 	rs = rb_rs;
 	if (rs == rb_default_rs) {
 	  smart_chomp:
-	    str_modify_keep_cr(str);
 	    enc = rb_enc_get(str);
 	    if (rb_enc_mbminlen(enc) > 1) {
 		pp = rb_enc_left_char_head(p, e-rb_enc_mbminlen(enc), e, enc);
@@ -6160,7 +6160,6 @@ rb_str_chomp_bang(int argc, VALUE *argv, VALUE str)
 		len--;
 	}
 	if (len < RSTRING_LEN(str)) {
-	    str_modify_keep_cr(str);
 	    STR_SET_LEN(str, len);
 	    RSTRING_PTR(str)[len] = '\0';
 	    return str;
@@ -6182,7 +6181,6 @@ rb_str_chomp_bang(int argc, VALUE *argv, VALUE str)
 	 memcmp(RSTRING_PTR(rs), pp, rslen) == 0)) {
 	if (rb_enc_left_char_head(p, pp, e, enc) != pp)
 	    return Qnil;
-	str_modify_keep_cr(str);
 	if (ENC_CODERANGE(str) != ENC_CODERANGE_7BIT) {
 	    ENC_CODERANGE_CLEAR(str);
 	}
@@ -6301,6 +6299,7 @@ rb_str_rstrip_bang(VALUE str)
     rb_encoding *enc;
     char *s, *t, *e;
 
+    str_modify_keep_cr(str);
     enc = STR_ENC_GET(str);
     rb_str_check_dummy_enc(enc);
     s = RSTRING_PTR(str);
@@ -6324,7 +6323,6 @@ rb_str_rstrip_bang(VALUE str)
     if (t < e) {
 	int len = t-RSTRING_PTR(str);
 
-	str_modify_keep_cr(str);
 	STR_SET_LEN(str, len);
 	RSTRING_PTR(str)[len] = '\0';
 	return str;