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>