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