# Code Review: MSX7

**URL:** <https://www.ruby-forum.com/t/code-review-msx7/148654>\
**Category:** IronRuby\
**Created:** [September 15, 2008, 10:35pm UTC](https://www.ruby-forum.com/t/code-review-msx7/148654 "2008-09-15T22:35:28Z")\
**Posts on this page:** 3\
**Page:** 1

<div class="post-metadata">

**Author:** ![grujildo](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/g/94ad74/32.png) [@grujildo](https://www.ruby-forum.com/u/grujildo)\
**Post date:** [September 15, 2008, 10:35pm UTC](https://www.ruby-forum.com/t/code-review-msx7/148654/1 "2008-09-15T22:35:28Z")

</div>

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

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

Tomas

---

<div class="post-metadata">

**Author:** ![grujildo](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/g/94ad74/32.png) [@grujildo](https://www.ruby-forum.com/u/grujildo)\
**Post date:** [September 15, 2008, 11:27pm UTC](https://www.ruby-forum.com/t/code-review-msx7/148654/2 "2008-09-15T23:27:41Z")

</div>

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?

---

<div class="post-metadata">

**Author:** ![grujildo](https://www.ruby-forum.com/letter_avatar_proxy/v4/letter/g/94ad74/32.png) [@grujildo](https://www.ruby-forum.com/u/grujildo)\
**Post date:** [September 16, 2008, 12:40am UTC](https://www.ruby-forum.com/t/code-review-msx7/148654/3 "2008-09-16T00:40:21Z")

</div>

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