Re: [ZLIB] Add support for windowBits, memLevel, raw (issue 130513003)

[ajoh...@google.com]

Adding @lrn for more comments on the API.

This is starting to look very nice :)


https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc
File runtime/bin/filter.cc (right):

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode70
runtime/bin/filter.cc:70: void
FUNCTION_NAME(Filter_CreateZLibInflate)(Dart_NativeArguments args) {
Please check out the stuff in DartUtils for this method, eg.
DartUtils::GetBooleanValue for raw_obj.

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode72
runtime/bin/filter.cc:72: Dart_Handle wBits_obj =
Dart_GetNativeArgument(args, 1);
wBits_obj -> window_bits_obj

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode93
runtime/bin/filter.cc:93: delete[] dictionary;
Double delete if filter failed to init?

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode100
runtime/bin/filter.cc:100: delete[] dictionary;
Double delete, the filter should have ownership here.

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode109
runtime/bin/filter.cc:109: if (Dart_IsError(Dart_BooleanValue(gzip_obj,
&gzip))) {
Not your code, but it would be great if you could do a cleanup here as
well, using DartUtils.

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode156
runtime/bin/filter.cc:156: delete[] dictionary;
Same memory issue here and below.

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode341
runtime/bin/filter.cc:341: // Either 0 Byte processed or either
// Either 0 bytes processed or error ?

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode346
runtime/bin/filter.cc:346: ZLibInflateFilter::~ZLibInflateFilter() {
Add delete dictionary_ here as well as in deflate destructor

https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#newcode423
runtime/bin/filter.cc:423: // Either 0 Byte processed or either
Comment.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart
File sdk/lib/io/data_transformer.dart (right):

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode9
sdk/lib/io/data_transformer.dart:9: const int ZLIB_MIN_WINDOW_BITS = 8;
Talking with lrn@ about this, can you create a new class, 'ZLib', and
but these constants into this?

That way we have a namespace for it, and if we get enums one day, it'll
be a non-breaking change.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode40
sdk/lib/io/data_transformer.dart:40: final ZLibCodec ZLIB = new
ZLibCodec();
The problem here is that ZLIB is now wrong coding style, it should be
zLib.

i'm fine with the errors being delayed until either encode(r)/decode(r)
is invoked.

https://codereview.chromium.org/130513003/

[l...@google.com]

Comments. Lots of comments. :)

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart
File sdk/lib/io/data_transformer.dart (right):

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode9
sdk/lib/io/data_transformer.dart:9: const int ZLIB_MIN_WINDOW_BITS = 8;

I think it is better with an abstract ZLib class containing the
constants.
It takes the same number of characters to write "ZLib." and "ZLIB_", and
it avoids polluting the top-level namespace of the entire dart:io
library.

BUT (capital!) I don't really want to have both "ZLib" and "ZLIB" in the
same scope. To avoid that, I'd prefer the wrapper class to be called
"ZLibOption", "ZLibFlag" or something similar, so you write ...
ZLibOption.MAX_WINDOW_BITS ...

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode20
sdk/lib/io/data_transformer.dart:20: const int ZLIB_DFT_LEVEL = 6;
Maybe DFT -> DEFAULT (if that's what it stands for)
Ditto above.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode40
sdk/lib/io/data_transformer.dart:40: final ZLibCodec ZLIB = new
ZLibCodec();

I don't *really* mind having ZLIB be a non-const value.
It is intended to be used as a non-changing object, it's just not a
compile-time constant. I can't see a good use for it at compile-time, so
I would be inclined to let this slip for backwards compatibility, if
there wasn't a better solution.

But, preferably, make a private const constructor:
const ZLibCodec ZLIB = const ZLibCodec._default();
which doesn't have to validate its inputs because it initializes fields
with the default trusted values directly:
const ZLibCodec._default()
: level = ZLIB_DFT_LEVEL,
windowBits = ZLIB_DFR_WINDOW_BITS,
...
raw = false;

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode54
sdk/lib/io/data_transformer.dart:54: * the default compression level.
Levels above `6` will have higher compression
long line.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode153
sdk/lib/io/data_transformer.dart:153: * the default compression level.
Levels above `6` will have higher compression
long line.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode165
sdk/lib/io/data_transformer.dart:165: * (1 << (windowBits + 2)) + (1
<< (memLevel + 9))
Indent by four spaces and put an empty line before this to make it a
"blockquote" code block.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode173
sdk/lib/io/data_transformer.dart:173: * STRATEGY_HUFFMAN_ONLY to force
Huffman encoding only (no string match), or
Use full names (ZLIB_*) for constants.
Use either [ZLIB_STRATEGY_DEFAULT] or `ZLIB_STRATEGY_DEFAULT` to refer
to the constant, preferably the former at least the first time it is
used in a doc-comment.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode204
sdk/lib/io/data_transformer.dart:204: this.windowBits:
ZLIB_DFT_WINDOW_BITS,
Arguably, I'd indent the lines so that "this" matches up with the one
above.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode221
sdk/lib/io/data_transformer.dart:221: dictionary: dictionary, raw: raw);
You can do the same const default constructor trick with ZLibEncoder
too, and return a const object here.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode242
sdk/lib/io/data_transformer.dart:242: * the default compression level.
Levels above `6` will have higher compression
long line.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode254
sdk/lib/io/data_transformer.dart:254: * (1 << (windowBits + 2)) + (1
<< (memLevel + 9))
Block-quote this.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode260
sdk/lib/io/data_transformer.dart:260: * Tunes the compression algorithm.
Use the value STRATEGY_DEFAULT for normal
Full constant references [ZLIB_STRATEGY_DEFAULT] (or, after introducing
ZLib: [Zlib.STATEGY_DEFAULT]).

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode319
sdk/lib/io/data_transformer.dart:319: * Start a chunked conversion using
the options given to the ZLibEncoder
No need to remove '[' and ']'.
If you don't want a link, change them to backquotes: `ZLibEncoder`. We
generally format all code-names as code.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode365
sdk/lib/io/data_transformer.dart:365: ZLibDecoder({this.windowBits:
ZLIB_DFT_WINDOW_BITS, this.dictionary: null,
Could have const default constructor too.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode526
sdk/lib/io/data_transformer.dart:526: if (windowBits is! int ||
I wouldn't test for int-ness here. The type annotation already says it
must be an int.
Just to be devious, I might change one (or both) of the following
comparisons to have the integer constant first, so any non-num value
that gets in here in production mode will crash and burn.

Ditto for the next two as well.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode529
sdk/lib/io/data_transformer.dart:529: throw new
ArgumentError("'windowBits' must be in range "
You can also use
new RangeError.range(windowBits, ZLIB_MIN_WINDOW_BITS,
ZLIB_MAX_WINDOW_BITS)
here.

https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transformer.dart#newcode553
sdk/lib/io/data_transformer.dart:553: var strategies =
<int>[ZLIB_STRATEGY_FILTERED, ZLIB_STRATEGY_HUFFMAN_ONLY,
Maybe make this list const. No need to allocate a new list every time
the function is called.

https://codereview.chromium.org/130513003/diff/370001/tests/standalone/io/zlib_test.dart
File tests/standalone/io/zlib_test.dart (right):

https://codereview.chromium.org/130513003/diff/370001/tests/standalone/io/zlib_test.dart#newcode35
tests/standalone/io/zlib_test.dart:35: Expect.listEquals([31, 139, 8, 0,
0, 0, 0, 0, 0, 3, 3, 0, 0, 0, 0, 0, 0, 0,
Long line.

https://codereview.chromium.org/130513003/

[l...@google.com]

Excellent!


https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transformer.dart
File sdk/lib/io/data_transformer.dart (right):

https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transformer.dart#newcode9
sdk/lib/io/data_transformer.dart:9: /// Minimal value for [windowBits]
There is no [windowBits] declaration in scope, so this would be a broken
link in dartdoc.
Maybe:
/// Minimal value for [ZLibCodec.windowBits], [ZLibEncoder.windowBits]
and [ZLibDecoder.windowBits].
Ditto below.

https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transformer.dart#newcode30
sdk/lib/io/data_transformer.dart:30: /// Compression strategies
This dartdoc only attaches to STRATEGY_FILTERED.
Either make it a non-dartdoc ("//" instead of "///") or put comments on
all the strategies (always preferable :).

https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transformer.dart#newcode67
sdk/lib/io/data_transformer.dart:67: * (1 << (windowBits + 2)) + (1
<< (memLevel + 9))
Empty line before and four-space indent to make this a blockquote.

https://codereview.chromium.org/130513003/

[l...@google.com]

Apart from indentation, LGTM!
What do you say, Anders?


https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transformer.dart
File sdk/lib/io/data_transformer.dart (right):

https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transformer.dart#newcode40
sdk/lib/io/data_transformer.dart:40: /// for data produced by a filter
(or predictor)
Preferably use entire sentences for comments (start with capital letter
and end with full stop).

https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transformer.dart#newcode76
sdk/lib/io/data_transformer.dart:76: * Specifies how much memory should
be allocated for the internal compression
Indentation is traditionally:
/**
*
(i.e., following starts below first star of comment starter).

https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transformer.dart#newcode81
sdk/lib/io/data_transformer.dart:81: *
This asterisk is correct, the rest should be indented one more.
(this was the line that made me notice it :)

Ditto for comments below.

https://codereview.chromium.org/130513003/

[ajoh...@google.com]

This is looking really nice! :)


https://codereview.chromium.org/130513003/diff/530001/runtime/bin/filter.cc
File runtime/bin/filter.cc (right):

https://codereview.chromium.org/130513003/diff/530001/runtime/bin/filter.cc#newcode128
runtime/bin/filter.cc:128: delete[] dictionary;
With the addition of 'delete[] dictionary_' in ~*Filter, this is now a
double free. Same below and above.

Change to

if (!filter->Init()) {
delete[] filter;
throw ...
}

as we in general don't validate allocations (new, malloc, ect). If
allocation fails, in most cases we have no way of recovering.

https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transformer.dart
File sdk/lib/io/data_transformer.dart (right):

https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transformer.dart#newcode8
sdk/lib/io/data_transformer.dart:8: abstract class ZLibConstant {
Add class comment. Also, have we considered:

- ZLibFlag
- ZLibOption
- ZLibValue

? I'm not sure how I feel about ZLibConstant :)

And, as this is pretty much an enum, should it be plural (+s)?

https://codereview.chromium.org/130513003/

[ajoh...@google.com]

LGTM! :)

https://codereview.chromium.org/130513003/

[Anders Johnsen]

I plan on landing this tomorrow, or the day after. :)

Cheers,

- Anders

On Fri, Feb 7, 2014 at 9:52 AM, <ajoh...@google.com> wrote:

LGTM! :)

https://codereview.chromium.org/130513003/