tfpt review "/shelveset:StringGsubVersion;REDMOND\dremy" Comment : This change fixes all of the gsub, each, and each_line specs. Added aditional tests to gsub spec. The excluded gsub tests are only failing on unimplemented string methods (lstrip!, rstrip!) Implemented simple mutablestring version that is used to assure that mutation hasn''t occurred during an iteration. tring#gsub with pattern and block sets $~ for access from the block String#gsub with pattern and block restores $~ after leaving the block String#gsub with pattern and block sets $~ to MatchData of last match and nil when there''s none for access from outside String#gsub with pattern and block raises a RuntimeError if the string is modified while substituting This is a revision on a previous shelfset I submitted for these changes. Based on feedback I did a few things differently. I didn''t do anything special with version it is just a simple property implementation at this point. Shri''s points about potential multi-threaded issues and overflow are well taken but it sounds like we will pick this up as requirements dictate. I did revise the way version gets incremented. Rather than creating a single function that both requires frozen and increments (didn''t someone say sometime that function should only do one thing ;)) I think this was a good thing, I ended up putting the Version++ in the logical place close to where a mutation actually occurs. This combined with putting tests for each mutation method caught some scenarios that would have failed. -------------- next part -------------- A non-text attachment was scrubbed... Name: StringGsubVersion.diff Type: application/octet-stream Size: 20202 bytes Desc: StringGsubVersion.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20080710/c728ce6f/attachment-0001.obj>
I think it would be better to increment the version inside MutableString class (not in -Ops). I can do that transition when I''ll revisit frozen/tainted flags on MutableString. So go ahead with your change. Tomas -----Original Message----- From: Dave Remy Sent: Thursday, July 10, 2008 6:21 PM To: IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: Code Review: StringGsubVersion tfpt review "/shelveset:StringGsubVersion;REDMOND\dremy" Comment : This change fixes all of the gsub, each, and each_line specs. Added aditional tests to gsub spec. The excluded gsub tests are only failing on unimplemented string methods (lstrip!, rstrip!) Implemented simple mutablestring version that is used to assure that mutation hasn''t occurred during an iteration. tring#gsub with pattern and block sets $~ for access from the block String#gsub with pattern and block restores $~ after leaving the block String#gsub with pattern and block sets $~ to MatchData of last match and nil when there''s none for access from outside String#gsub with pattern and block raises a RuntimeError if the string is modified while substituting This is a revision on a previous shelfset I submitted for these changes. Based on feedback I did a few things differently. I didn''t do anything special with version it is just a simple property implementation at this point. Shri''s points about potential multi-threaded issues and overflow are well taken but it sounds like we will pick this up as requirements dictate. I did revise the way version gets incremented. Rather than creating a single function that both requires frozen and increments (didn''t someone say sometime that function should only do one thing ;)) I think this was a good thing, I ended up putting the Version++ in the logical place close to where a mutation actually occurs. This combined with putting tests for each mutation method caught some scenarios that would have failed.
Ok, cool. Sounds good. Are you thinking of a method in MutableString like IncrementVersion()? Or something? -----Original Message----- From: Tomas Matousek Sent: Thursday, July 10, 2008 6:46 PM To: Dave Remy; IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: RE: Code Review: StringGsubVersion I think it would be better to increment the version inside MutableString class (not in -Ops). I can do that transition when I''ll revisit frozen/tainted flags on MutableString. So go ahead with your change. Tomas -----Original Message----- From: Dave Remy Sent: Thursday, July 10, 2008 6:21 PM To: IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: Code Review: StringGsubVersion tfpt review "/shelveset:StringGsubVersion;REDMOND\dremy" Comment : This change fixes all of the gsub, each, and each_line specs. Added aditional tests to gsub spec. The excluded gsub tests are only failing on unimplemented string methods (lstrip!, rstrip!) Implemented simple mutablestring version that is used to assure that mutation hasn''t occurred during an iteration. tring#gsub with pattern and block sets $~ for access from the block String#gsub with pattern and block restores $~ after leaving the block String#gsub with pattern and block sets $~ to MatchData of last match and nil when there''s none for access from outside String#gsub with pattern and block raises a RuntimeError if the string is modified while substituting This is a revision on a previous shelfset I submitted for these changes. Based on feedback I did a few things differently. I didn''t do anything special with version it is just a simple property implementation at this point. Shri''s points about potential multi-threaded issues and overflow are well taken but it sounds like we will pick this up as requirements dictate. I did revise the way version gets incremented. Rather than creating a single function that both requires frozen and increments (didn''t someone say sometime that function should only do one thing ;)) I think this was a good thing, I ended up putting the Version++ in the logical place close to where a mutation actually occurs. This combined with putting tests for each mutation method caught some scenarios that would have failed.
The version updates would be internal detail of MutableString. You would be able to ask for the current version from other classes, but not change it. The operations on MutableString will do. Tomas -----Original Message----- From: Dave Remy Sent: Thursday, July 10, 2008 6:51 PM To: Tomas Matousek; IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: RE: Code Review: StringGsubVersion Ok, cool. Sounds good. Are you thinking of a method in MutableString like IncrementVersion()? Or something? -----Original Message----- From: Tomas Matousek Sent: Thursday, July 10, 2008 6:46 PM To: Dave Remy; IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: RE: Code Review: StringGsubVersion I think it would be better to increment the version inside MutableString class (not in -Ops). I can do that transition when I''ll revisit frozen/tainted flags on MutableString. So go ahead with your change. Tomas -----Original Message----- From: Dave Remy Sent: Thursday, July 10, 2008 6:21 PM To: IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: Code Review: StringGsubVersion tfpt review "/shelveset:StringGsubVersion;REDMOND\dremy" Comment : This change fixes all of the gsub, each, and each_line specs. Added aditional tests to gsub spec. The excluded gsub tests are only failing on unimplemented string methods (lstrip!, rstrip!) Implemented simple mutablestring version that is used to assure that mutation hasn''t occurred during an iteration. tring#gsub with pattern and block sets $~ for access from the block String#gsub with pattern and block restores $~ after leaving the block String#gsub with pattern and block sets $~ to MatchData of last match and nil when there''s none for access from outside String#gsub with pattern and block raises a RuntimeError if the string is modified while substituting This is a revision on a previous shelfset I submitted for these changes. Based on feedback I did a few things differently. I didn''t do anything special with version it is just a simple property implementation at this point. Shri''s points about potential multi-threaded issues and overflow are well taken but it sounds like we will pick this up as requirements dictate. I did revise the way version gets incremented. Rather than creating a single function that both requires frozen and increments (didn''t someone say sometime that function should only do one thing ;)) I think this was a good thing, I ended up putting the Version++ in the logical place close to where a mutation actually occurs. This combined with putting tests for each mutation method caught some scenarios that would have failed.