Changes are largely good. I have the following questions, issues and comments:
1. In StringFormatter, "tainted?" is called for every argument, even if a previous call already returned true. Is that what MRI does, or does it optimize by only calling tainted? when the string being built is still untainted? 2. MutableString.IndexOf(byte value) is inconsistent with the other versions of IndexOf; it makes an extra intermediate call. 3. MutableString.LastIndexOf(byte[] value) and MutableString.LastIndexOf(MutableString value) calculate the starting position inconsistently with the other forms of LastIndexOf. They use (length - 1) instead of (length - value.Length). 4. What's going to happen to the commented-out calls to RequireArrayRange in MutableString.cs? 5. In Tokenizer, shouldn't the (uint) conversions all be "unchecked"? (I can't remember any more which way we decided on explicit "unchecked" in the Ruby source.) 6. The not-in-place BlockReplace functionality in MutableStringOps no longer checks to see if the block tried to mutate the string because it makes a copy before calling Yield. I seem to recall that the old way was more consistent with MRI even though the new way arguably makes more sense. Am I just misremembering that this applied to both the in-place and not-in-place operations, and that MRI only does this for the in-place operation? -----Original Message----- From: Tomas Matousek Sent: Montag, 15. September 2008 13:24 To: IronRuby External Code Reviewers Cc: [email protected] Subject: Code Review: MSX7 tfpt review "/shelveset:MSX7;REDMOND\tomat" More string work. Implements versioning of MutableStrings, several missing methods and fixes some bugs. Tomas _______________________________________________ Ironruby-core mailing list [email protected] http://rubyforge.org/mailman/listinfo/ironruby-core
