[Ironruby-core] Code Review: MSX7

Tomas Matousek Tomas.Matousek at microsoft.com
Mon Sep 15 18:33:18 EDT 2008


1) It's actually a dead code :)
2) I'll fix the others to make the intermediate calls as well (they'll get inlined so no need for code duplication).
3) All should use length - 1, will fix.
4) Don't know yet where do exactly we want to check (if we do want to check at all). Will see.
5) Yes, they should. Will fix.
6) The new way is actually more consistent with MRI. There were specs failing (the block can change the string in sub w/o raising an exception).

Tomas


-----Original Message-----
From: Curt Hagenlocher
Sent: Monday, September 15, 2008 2:19 PM
To: Tomas Matousek; IronRuby External Code Reviewers
Cc: ironruby-core at rubyforge.org
Subject: RE: Code Review: MSX7

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: ironruby-core at rubyforge.org
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


More information about the Ironruby-core mailing list