[Ironruby-core] Code Review: zlib

John Lam (IRONRUBY) jflam at microsoft.com
Thu May 1 12:30:10 EDT 2008


Hi Michael,

Thanks so much for sending in this patch! I did a high-level review of the code (I didn't attempt to analyze the algorithms that you're using) for compatibility with the rest of the codebase, style, invariants etc.

I'm attaching a diff to this mail so that folks can follow along with the discussion. Note that the diff isn't quite as useful as it might be in this case because the code formatting was different on the way in.

In the future, we can do a more detailed review of the algorithms but for now let's just treat it as a black box.

Overall I think it's awesome that it works as advertised. There's just some work that needs to be done to make sure that the methods follow Ruby semantics.

I intend to check this code in as-is for the time being since we don't have any tests that exercise it in our current test matrix. This should make it easier for folks to work on it instead of shipping around patch files on mailing lists. ETA should be tomorrow on this check-in.


General comments:
=================

1. Attach our standard MsPL copyright header to the top of files
2. Follow standard .NET naming conventions (PascalCaseMethodsAndClasses, _instanceMembers). This is really easy to fix after the fact via VS refactoring tools, but it's a matter of who does the work :)
3. Please spend some time thinking about your invariants, and in particular your non-nullable reference types (that's what the /*!*/ annotations mean). This will help you think about cases where you need to add explicit null checks vs. cases where you don't.
4. Consider using #region to group chunks of code together
5. If there's more work or missing work, a few TODO comments would help guide folks reading the code.
6. PLEASE format your code the way the rest of the source code is formatted, since this will make diffs on code reviews much more readable (there's *way* too much noise in this diff).

Specific comments:
==================

1. Using "initialize" to initialize your .NET invariants is a bad idea - someone could override initialize and invalidate your invariants. Move those things to a constructor instead.

Instead of:

            [RubyMethod("initialize")]
            public static ZStream/*!*/ Initialize(ZStream/*!*/ self) {
                self._outPos = -1;
                self._inPos = -1;
                self._bitBucket = 0;
                self._bitCount = 0;
                self._inputBuffer = new List<byte>();
                self._outputBuffer = new List<byte>();
                return self;
            }

Do:

            public ZStream() {
                _outPos = -1;
                _inPos = -1;
                _bitBucket = 0;
                _bitCount = 0;
                _inputBuffer = new List<byte>();
                _outputBuffer = new List<byte>();
            }

2. The HuffmanTree class:

a) does not appear to be a Ruby class
b) should use properties with backing private fields

3. The GzipReader#open instance method:

This appears to be a private instance method in Ruby. You had it defined as a public instance method that creates a new instance of GZipReader. I doubt that this is correct. I suspect that it is used to initialize the current instance of GZipReader to the IO object that is passed in.

I believe that the code that you have in the GZipReader's constructor should be moved to this method.

4. The GzipReader.open singleton methods:

You only have a single overload that handles the MutableString case. In most cases, Ruby libraries will do an implicit conversion via to_str on any arbitrary object that is passed in as a parameter. You'll need to use the [NotNull] attribute to force the call to the overload that accepts object for the case where the caller passes in nil.

Instead of:

public static GZipReader Open(CodeContext/*!*/ context, RubyClass/*!*/ self, MutableString/*!*/ filename) { ... }

Do

public static GZipReader Open(CodeContext/*!*/ context, RubyClass/*!*/ self, [NotNull]MutableString/*!*/ filename) { ... }

public static GZipReader Open(CodeContext/*!*/ context, RubyClass/*!*/ self, object  filename) {
  return Open(context, self, Protocols.CastToString(context, filename));
}

5. The block method overload of GzipReader.open()

The signature wasn't correct - the correct signature is:

public static void Open(CodeContext/*!*/ context, RubyClass/*!*/ self, BlockParam block, [NotNull]MutableString/*!*/ filename)

There were a few other problems in the original implementation

a) allocate just allocates memory - it doesn't call initialize to initialize a new instance of a specific class; you'll need to invoke new on that class.
b) open is a private instance method, so you won't need to invoke it via a site

Tests:
======

I still have to sync to the latest rubinius specs which has a spec suite for zlib.

Thanks,
-John
-------------- next part --------------
A non-text attachment was scrubbed...
Name: zlib.diff
Type: application/octet-stream
Size: 96506 bytes
Desc: zlib.diff
URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20080501/cab74f7b/attachment-0001.obj>


More information about the Ironruby-core mailing list