Code Review: MSX7

Ruby IronRuby
1

tfpt review “/shelveset:MSX7;REDMOND\tomat”

More string work. Implements versioning of MutableStrings, several
missing methods and fixes some bugs.

Tomas

2

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?

3
  1. It’s actually a dead code :slight_smile:
  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