Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
130513003:
[ZLIB] Add support for windowBits, memLevel, raw (Closed)
|
Created:
6 years, 11 months ago by vicb Modified:
6 years, 9 months ago CC:
reviews_dartlang.org, vm-dev_dartlang.org Visibility:
Public. |
Description[ZLIB] Add support for windowBits, memLevel, raw
BUG=159367
**BC breaks:**
All public (API) classes that used to be const are not const any more. This allow checking the validity of input parameters in the constructor body.
- ZLibCodec,
- GZipCodec,
- ZLibEncoder,
- ZLibDecoder
The globals codecs GZIP and ZLIB are now final we should be ok (the class contain only final properties, ie no state).
What should be done:
(- finer grained exceptions, -> later change)
(- Handle the flush modes but probably in an other patch as the updates might be more complex.)
Done:
Patch Set #1
- add some more docs,
- support for memLevel.
Patch Set #2:
- implement dictionaries (todo: fix mem management),
- integrate review feedback,
- fix CC,
- rebased on master
Patch Set #4:
- support for strategy,
- add some checks (ie checking the level, memLevel, ...) this could imply adding a constructor body (Codecs, Coders) but this would force making the const non const and break BC, thoughts ?
- Add ZLIB constants Dart side.
- Refactor the tests and test only some levels rather all which was unit testing rather more than the extension. It saves some test time.
Patch Set #8:
- fix mem management for dictionaries,
R=ajohnsen@google.com, lrn@google.com
Committed: https://code.google.com/p/dart/source/detail?r=33206
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add support for memLevel #
Total comments: 11
Patch Set 3 : rebase, integrate feedback, fix cc #Patch Set 4 : constants, checks, strategy #Patch Set 5 : Almost done, need help #
Total comments: 36
Patch Set 6 : integrate Anders feedback #Patch Set 7 : memory management #Patch Set 8 : fix dictionary memory management #
Total comments: 49
Patch Set 9 : integrate feedback #Patch Set 10 : fix previous CL #Patch Set 11 : should fix the mess, sorry ! #
Total comments: 6
Patch Set 12 : integrate feedback #
Total comments: 9
Patch Set 13 : integrate feedback #
Messages
Total messages: 23 (0 generated)
https://codereview.chromium.org/130513003/diff/1/sdk/lib/io/data_transformer.... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/1/sdk/lib/io/data_transformer.... sdk/lib/io/data_transformer.dart:53: const ZLibCodec({this.level: 6, this.windowBits: 15, this.raw: false}); Adding some checks here would imply removing the 'const' and break BC
One question that I have is how to "share" constants (ie C #define) across c++ and dart ? Is there any good way to do that or should I duplicate constant names in a Dart class and use some kind of mapping function (assuming dart values that we've created won't change but foreign c++ defines might) ?
Anders, I have the support for dictionary that is almost ready. I am still not satisfied with the memory management part of it (releasing at multiple places, ie before throwing). Is there some kind of smart pointer helpers in the C++ Dart code base that could be used to help ? Cheers, Victor
Overall, this is looking nice. Thank you for taking the time to also add more tests! :) https://codereview.chromium.org/130513003/diff/70001/runtime/bin/filter.h File runtime/bin/filter.h (right): https://codereview.chromium.org/130513003/diff/70001/runtime/bin/filter.h#new... runtime/bin/filter.h:57: ZLibDeflateFilter(bool gzip = false, int level = 6, int windowBits = 15, Do the arguments need a default value here (and below)? https://codereview.chromium.org/130513003/diff/70001/runtime/bin/filter.h#new... runtime/bin/filter.h:85: ZLibInflateFilter(int windowBits = 15, bool raw = false) : Move : to next line. https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... sdk/lib/io/data_transformer.dart:68: const ZLibCodec({this.level: 6, this.windowBits: 15, this.raw: false, Please add some constants to ZLibCodec, ZLIB_LEVEL_DEFAULT, ZLIB_LEVEL_MIN, etc. https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... sdk/lib/io/data_transformer.dart:68: const ZLibCodec({this.level: 6, this.windowBits: 15, this.raw: false, Make raw the last argument.
Thanks for the review. https://codereview.chromium.org/130513003/diff/70001/runtime/bin/filter.h File runtime/bin/filter.h (right): https://codereview.chromium.org/130513003/diff/70001/runtime/bin/filter.h#new... runtime/bin/filter.h:57: ZLibDeflateFilter(bool gzip = false, int level = 6, int windowBits = 15, On 2014/01/15 10:52:08, Anders Johnsen wrote: > Do the arguments need a default value here (and below)? Nope, you're right, will update https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... sdk/lib/io/data_transformer.dart:68: const ZLibCodec({this.level: 6, this.windowBits: 15, this.raw: false, On 2014/01/15 10:52:08, Anders Johnsen wrote: > Please add some constants to ZLibCodec, ZLIB_LEVEL_DEFAULT, ZLIB_LEVEL_MIN, etc. > That's in the pipe https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... sdk/lib/io/data_transformer.dart:68: const ZLibCodec({this.level: 6, this.windowBits: 15, this.raw: false, On 2014/01/15 10:52:08, Anders Johnsen wrote: > Make raw the last argument. Could I ask why ? (is order important for named params)
https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... sdk/lib/io/data_transformer.dart:68: const ZLibCodec({this.level: 6, this.windowBits: 15, this.raw: false, On 2014/01/15 11:08:04, vicb wrote: > On 2014/01/15 10:52:08, Anders Johnsen wrote: > > Make raw the last argument. > > Could I ask why ? (is order important for named params) Matter of taste I suppose :) `raw` is kind of different than the other flags, so I think it should be in the front or back. And given that level is the most common one, I suggested in the back. It's purely a suggestion, and you are free to leave it as it, if you prefer.
Anders, I have rebase on latest master, integrated your feedback and fix some CC. I'll update the description with what is left to do. I should have some spare time today to fix the remaining issues / todo. Cheers, Victor https://codereview.chromium.org/130513003/diff/70001/runtime/bin/filter.h File runtime/bin/filter.h (right): https://codereview.chromium.org/130513003/diff/70001/runtime/bin/filter.h#new... runtime/bin/filter.h:57: ZLibDeflateFilter(bool gzip = false, int level = 6, int windowBits = 15, fixed https://codereview.chromium.org/130513003/diff/70001/runtime/bin/filter.h#new... runtime/bin/filter.h:85: ZLibInflateFilter(int windowBits = 15, bool raw = false) : no more relevant https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/70001/sdk/lib/io/data_transfor... sdk/lib/io/data_transformer.dart:68: const ZLibCodec({this.level: 6, this.windowBits: 15, this.raw: false, I've moved raw to be the last parameter everywhere
Anders, I am almost done with this patch, but I can not figure out a good way to allocate the dictionary. Would you have some time to give me some hints ? The version of the code I pushed does not work (because of the mem management). Thanks, Victor
Here's the first round of comments. I hope this can help you solve your problems with memory :) Cheers, - Anders https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc File runtime/bin/filter.cc (right): https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:38: Dart_Handle AllocateDictionary(Dart_Handle dictionary_obj) { static https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:44: printf("+allocate"); Please remove debug prints in this file. https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:51: Dart_Handle dictionary = IOBuffer::Allocate(length, &dst); Wait with allocation until you have src? https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:88: dictionary = AllocateDictionary(dict_obj); Why are you doing this? :) Would it make sense to either call it CopyTypedList or use create a persistent handle instead? https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:286: window_bits *= -1; = -window_bits; https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:299: if (dictionary_ != NULL && !gzip_ && !raw_) { !Dart_IsNull(directory_) https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:303: Dart_Handle handle = Dart_TypedDataAcquireData(dictionary_, &type, Place all arguments on each line, with 4 indentation. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:420: Dart_Handle handle = Dart_TypedDataAcquireData(dictionary_, &type, Indentation of arguments. https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.h File runtime/bin/filter.h (right): https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.h#ne... runtime/bin/filter.h:75: const Dart_Handle dictionary_; This should be a Dart_PersistentHandle. Checkout Dart_NewPersistentHandle and Dart_DeletePersistentHandle. https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.h#ne... runtime/bin/filter.h:98: const Dart_Handle dictionary_; Ditto. https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter_patc... File runtime/bin/filter_patch.dart (right): https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter_patc... runtime/bin/filter_patch.dart:17: print("Dart _ZLibInflateFilter $windowBits $dictionary $raw"); Debug stuff. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:9: const int ZLIB_MIN_WINDOW_BITS = 8; If these are moved to ZLibCodec as final members, we can say ZLIB.MIN_WINDOW_BITS instead. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:40: final ZLibCodec ZLIB = new ZLibCodec(); make const. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:110: validateZLibeLevel(level); Move checks to encoder and decoder getters, or better yet, ZLibEncoder/Decoder constructor. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:193: * default empty dictionary. Maybe write some more about lifeness - do we take a copy or do we just expect the list to stay unchanged. Also, the type is here List<int>. That can be any lists of ints. Look into using _ensureFastAndSerializableByteData for this list, when passing on to native. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:288: <<<<<<< HEAD Merge stuff. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:331: print("Dart _ZLibEncoderSink"); Debug stuff. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:528: void validateZLibWindowBits(int windowBits) { Make these functions private. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:530: windowBits < ZLIB_MIN_WINDOW_BITS || windowBits > ZLIB_MAX_WINDOW_BITS) { Move last test to next line, as well as 2 places below. https://codereview.chromium.org/130513003/diff/250001/tests/standalone/io/zli... File tests/standalone/io/zlib_test.dart (right): https://codereview.chromium.org/130513003/diff/250001/tests/standalone/io/zli... tests/standalone/io/zlib_test.dart:226: // testZLibDeflateEmptyGzip(); uncommented tests. Please add some dictionary tests as well :)
Anders, It didn't help me much but I'll try to take an other look now, sometimes only letting things alone for a few days helps. Note that I'll remove all the debug stuff when I'm done debugging this :) One last question: I have a few other patches pending (https://codereview.chromium.org/search?closed=1&owner=victor.berchet%40gmail.com) is there anything I missed to get them reviewed or is the team busy and only time is needed ? Thanks for your help, Victor https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc File runtime/bin/filter.cc (right): https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:38: Dart_Handle AllocateDictionary(Dart_Handle dictionary_obj) { fixed (and updated the 2 above functions) https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:51: Dart_Handle dictionary = IOBuffer::Allocate(length, &dst); I need dst in both if and else branches... https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:88: dictionary = AllocateDictionary(dict_obj); I want to get a snapshot of the dict on creation to that it could be released after or further updates would not modify the content. https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:286: window_bits *= -1; fixed https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:299: if (dictionary_ != NULL && !gzip_ && !raw_) { Sure ? We have "Dart_Handle dictionary = NULL;" in both Filter_CreateZLibInflate and Filter_CreateZLibDeflate. https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:303: Dart_Handle handle = Dart_TypedDataAcquireData(dictionary_, &type, fixed https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:420: Dart_Handle handle = Dart_TypedDataAcquireData(dictionary_, &type, fixed https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.h File runtime/bin/filter.h (right): https://codereview.chromium.org/130513003/diff/250001/runtime/bin/filter.h#ne... runtime/bin/filter.h:75: const Dart_Handle dictionary_; The thing is IOBuffer::Allocate returns a Dart_Handle. This also leads to one question I had about this code, should it rather reads like: Dart_Handle IOBuffer::Allocate(intptr_t size, uint8_t **buffer) { uint8_t* data = Allocate(size); Dart_Handle result = Dart_NewExternalTypedData( Dart_TypedData_kUint8, data, size); **result =**Dart_NewWeakPersistentHandle(result, data, IOBuffer::Finalizer); if (Dart_IsError(result)) { Free(data); Dart_PropagateError(result); } if (buffer != NULL) { *buffer = data; } return result; } Or should I turn the returned handle to a persistent one by hand ? https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:9: const int ZLIB_MIN_WINDOW_BITS = 8; But in the doc they'll be listed in ZLibCodec, not sure it's a good thing ? https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:40: final ZLibCodec ZLIB = new ZLibCodec(); Can not because of the required checks https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:110: validateZLibeLevel(level); As the decoder & encoder are lazily instantiated the error would not trigger before actually decoding / encoding which I think is not good. What's the downside of having ZLIB final (instead of const) ? https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:193: * default empty dictionary. I tool models on the current converters. However the base converter is generic, I'll check _ensureFastAndSerializableByteData and update accordingly. https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:288: <<<<<<< HEAD fixed https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:528: void validateZLibWindowBits(int windowBits) { fixed https://codereview.chromium.org/130513003/diff/250001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:530: windowBits < ZLIB_MIN_WINDOW_BITS || windowBits > ZLIB_MAX_WINDOW_BITS) { fixed https://codereview.chromium.org/130513003/diff/250001/tests/standalone/io/zli... File tests/standalone/io/zlib_test.dart (right): https://codereview.chromium.org/130513003/diff/250001/tests/standalone/io/zli... tests/standalone/io/zlib_test.dart:226: // testZLibDeflateEmptyGzip(); I'll remove debug stuffs when I'm done. The dictionary test is the last & uncommented one.
Anders, The memory management is now fixed, could you please take a look at it ? I would have preferred using a Weak handle but I think IOBuffer::allocate has an issue, see https://code.google.com/p/dart/issues/detail?id=16388 There is still an open item for the const constructor. I think final is good enough and enable nicer error management. Waiting for your feedback, Cheers, Victor
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#n... 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#n... 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#n... 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#n... 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#n... 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#n... runtime/bin/filter.cc:156: delete[] dictionary; Same memory issue here and below. https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#n... 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#n... 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#n... runtime/bin/filter.cc:423: // Either 0 Byte processed or either Comment. https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... 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_transfo... 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.
Comments. Lots of comments. :) https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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_transfo... 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/zli... File tests/standalone/io/zlib_test.dart (right): https://codereview.chromium.org/130513003/diff/370001/tests/standalone/io/zli... 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.
No problem for the number / length of comments, quite the opposite actually, I appreciate the constructive feedback. All issues should be fixed now. Sorry for the mess I did trying to upload the correct version... the latest CL should be ok. Thanks ! 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#n... runtime/bin/filter.cc:70: void FUNCTION_NAME(Filter_CreateZLibInflate)(Dart_NativeArguments args) { I'll update https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:72: Dart_Handle wBits_obj = Dart_GetNativeArgument(args, 1); done https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:93: delete[] dictionary; we throw after delete[] https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:100: delete[] dictionary; we throw after delete[] https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:156: delete[] dictionary; same answer https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:341: // Either 0 Byte processed or either oops, right ! https://codereview.chromium.org/130513003/diff/370001/runtime/bin/filter.cc#n... runtime/bin/filter.cc:346: ZLibInflateFilter::~ZLibInflateFilter() { will do. safe as set to null when destroy as part of the normal processing https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:9: const int ZLIB_MIN_WINDOW_BITS = 8; This is what I did first before reverting. I'll revert again :) https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:9: const int ZLIB_MIN_WINDOW_BITS = 8; Changed for ZLibConst + DEFAULT_ https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:40: final ZLibCodec ZLIB = new ZLibCodec(); ZLIB is "almost constant" as the class has no state. Deferring errors is worse than having "ZLIB", don't you think ? https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:40: final ZLibCodec ZLIB = new ZLibCodec(); done https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:54: * the default compression level. Levels above `6` will have higher compression fixed https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:204: this.windowBits: ZLIB_DFT_WINDOW_BITS, I prefer to let my IDE decide and not reindent by hand on each change if it's ok with you (CS are 4+ spaces). https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:254: * (1 << (windowBits + 2)) + (1 << (memLevel + 9)) fixed https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:260: * Tunes the compression algorithm. Use the value STRATEGY_DEFAULT for normal fixed https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:319: * Start a chunked conversion using the options given to the ZLibEncoder fixed https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:526: if (windowBits is! int || Good to know, fixed https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:529: throw new ArgumentError("'windowBits' must be in range " Thanks for the tip https://codereview.chromium.org/130513003/diff/370001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:553: var strategies = <int>[ZLIB_STRATEGY_FILTERED, ZLIB_STRATEGY_HUFFMAN_ONLY, done https://codereview.chromium.org/130513003/diff/370001/tests/standalone/io/zli... File tests/standalone/io/zlib_test.dart (right): https://codereview.chromium.org/130513003/diff/370001/tests/standalone/io/zli... 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, fixed
Excellent! https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transfo... 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_transfo... 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_transfo... 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.
every pending reviews should be fixed in the latest commit. https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:9: /// Minimal value for [windowBits] fixed https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:30: /// Compression strategies fixed https://codereview.chromium.org/130513003/diff/490001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:67: * (1 << (windowBits + 2)) + (1 << (memLevel + 9)) fixed
Apart from indentation, LGTM! What do you say, Anders? https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transfo... 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_transfo... 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_transfo... 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.
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#n... 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_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transfo... 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)?
Anders, Lasse, Thank you both for youe patience and very valuable feedback. I have integrated your comments in the latest commit. 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#n... runtime/bin/filter.cc:128: delete[] dictionary; fixed https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transfo... File sdk/lib/io/data_transformer.dart (right): https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:8: abstract class ZLibConstant { fixed. I really like "ZLibOption", changed for it. https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:40: /// for data produced by a filter (or predictor) fixed https://codereview.chromium.org/130513003/diff/530001/sdk/lib/io/data_transfo... sdk/lib/io/data_transformer.dart:76: * Specifies how much memory should be allocated for the internal compression that's must be IntelliJ playing tricks on me... fixed everywhere
LGTM! :)
Message was sent while issue was closed.
Committed patchset #13 manually as r33206 (presubmit successful).
Message was sent while issue was closed.
FYI, sizeof(dictionary_) will return the pointer width, not the size of the dictionary :) Fixed in a follow-up.
Message was sent while issue was closed.
On 2014/03/03 13:19:04, Anders Johnsen wrote: > FYI, sizeof(dictionary_) will return the pointer width, not the size of the > dictionary :) Fixed in a follow-up. Oops... of course ! Thanks for catching (& fixing) this and merging this CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
