Re: Code Review: zlib

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 :slight_smile:
  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>();
        }
  1. The HuffmanTree class:

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

  1. 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.

  1. 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));
}

  1. 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

This forum is not affiliated to the Ruby language, Ruby on Rails framework, nor any Ruby applications discussed here.

| Privacy Policy | Terms of Service | Remote Ruby Jobs