Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12534011:
Add base64 decoder and change existing base64 encoder. (Closed)
|
Created:
7 years, 9 months ago by mdakin1 Modified:
7 years, 8 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionAdd base64 decoder and change existing base64 encoder.
New implmentation supports url/file safe encoding and should be faster.
Committed: https://code.google.com/p/dart/source/detail?r=20845
Patch Set 1 #Patch Set 2 : minor fix #
Total comments: 68
Patch Set 3 : Address comments. #Patch Set 4 : Address comments. #
Total comments: 38
Patch Set 5 : Address all comments from Florian and Lasse. #
Total comments: 8
Patch Set 6 : Minor fixes. #Patch Set 7 : Change iterations in perf test. #Patch Set 8 : Minor fixes, make encoder padding bits more consistent with older version. #Patch Set 9 : Remove whitespace #
Messages
Total messages: 8 (0 generated)
Preliminary comments, not done yet. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... File sdk/lib/crypto/crypto_base.dart (right): https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:127: * base64 encoded string optionally broken up in to lines of 76 Try to have the first line of the doc explain the high-level operation of the method, as a single sentence. This is the line that will be initially visible in the generated DartDoc. Other general rules for dartdoc: Avoid abbreviations ("characters", not "chars"). Use full sentences (you do). In this case, for example: /** * Converts a list of bytes into a Base 64 encoded string. * * The list can be any list of integers in the range 0..255, * for example a message digest. * * If [addLineSeparator] is true, the resulting string will be * broken into lines of 76 characters, separated by "\r\n". * * If [urlSafe] is true, the result is URL and filename safe. * * It is an error if both [urlSafe] and [addSeparator] are true. * * Based on RFC 4648 - http://tools.ietf.org/html/tfc4648 */ https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:131: * Based on RFC 4686 http://tools.ietf.org/html/rfc4648 4686 -> 4648 https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:140: * Converts a base64 encoded String into list of bytes. Decoder ignores \r\n base64 -> Base64. into list -> into a list New paragraph after "bytes.". https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:141: * from input. By default also ignores all illegal chars unless \r\n from inpu -> "\r\n" sequences in the input. By default also -> By default it also chars -> characters. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:142: * [ignoreErrors] is false. New paragraph here. Dartdoc flows like HTML, so the newline here won't matter, but a second one will start a new paragraph. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:143: * Accepts both url safe and unsafe base64 encoded strings. url -> URL base64->Base 64 (just my preference, stands out more if capitalized). https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:144: * Returns empty list if [input] is null. Returns empty -> Returns an empty https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:145: * Based on RFC 4686 http://tools.ietf.org/html/rfc4648 4686 -> 4648 https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:21: "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/".codeUnits; Consider not extracting code units here, and just call 'codeUnitAt(i)' where you use it. The codeUnits object is just a wrapper around the string, and it does exactly the same thing, just with more overhead. Do add a type to the declaration (String or List<int> depending on the above). https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:24: "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_".codeUnits; Ditto. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:26: static const List<int> _decodeTable = Add a comment, it doesn't have to be dartdoc, to explain the -2 and -1. I can guess it, but it's still better to not have to guess. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:31: -2, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, Consider padding with spaces here so every number has the same size (and yes, it requires re-breaking every line after here :) https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:44: static String bytesToBase64(List<int> bytes, [bool urlSafe, bool addLineSeparator]) { Long line needs to be broken. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:45: int len = bytes != null ? bytes.length : 0; Consider not accepting null. It may just be hiding an error. The caller can check for null if he knows it's possible and just not call at all in that case. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:51: final int cLen = len ~/ 3 * 3; cLen is a non-obvious abbreviated name. Consider giving it a longer name (characterLength, chunkCount for just "len ~/ 3" or similar). https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:52: final int padLen = len - cLen; padLength. Is this done this way because it's faster than int padLen = len % 3; int cLen = len - padLen; ? I wouldn't be surprised if it is. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:57: oLen += (oLen - 1) ~/ 76 << 1; Parentheses around the left-hand-side of <<. Nobody remembers whether ~/ or << has higher priority :) https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:79: int x = bytes[cLen] << 10 | (padLen == 2 ? bytes[len - 1] << 2 : 0); More parentheses! Does << bind harder than | ? Who'd have thunk?! There's an extra space after 10.
Thanks, I addressed most of the comments. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... File sdk/lib/crypto/crypto_base.dart (right): https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:127: * base64 encoded string optionally broken up in to lines of 76 On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Try to have the first line of the doc explain the high-level operation of the > method, as a single sentence. This is the line that will be initially visible in > the generated DartDoc. > > Other general rules for dartdoc: Avoid abbreviations ("characters", not > "chars"). Use full sentences (you do). > > In this case, for example: > /** > * Converts a list of bytes into a Base 64 encoded string. > * > * The list can be any list of integers in the range 0..255, > * for example a message digest. > * > * If [addLineSeparator] is true, the resulting string will be > * broken into lines of 76 characters, separated by "\r\n". > * > * If [urlSafe] is true, the result is URL and filename safe. > * > * It is an error if both [urlSafe] and [addSeparator] are true. > * > * Based on RFC 4648 - http://tools.ietf.org/html/tfc4648 > */ Done, > * It is an error if both [urlSafe] and [addSeparator] are true. Instead of making it an error, should we just ignore the addSeparator if urlSafe is true? https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:131: * Based on RFC 4686 http://tools.ietf.org/html/rfc4648 On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > 4686 -> 4648 Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:140: * Converts a base64 encoded String into list of bytes. Decoder ignores \r\n On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > base64 -> Base64. > into list -> into a list > New paragraph after "bytes.". Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:141: * from input. By default also ignores all illegal chars unless On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > \r\n from inpu -> "\r\n" sequences in the input. > By default also -> By default it also > chars -> characters. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:142: * [ignoreErrors] is false. On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > New paragraph here. Dartdoc flows like HTML, so the newline here won't matter, > but a second one will start a new paragraph. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:143: * Accepts both url safe and unsafe base64 encoded strings. On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > url -> URL > base64->Base 64 (just my preference, stands out more if capitalized). Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:144: * Returns empty list if [input] is null. On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Returns empty -> Returns an empty Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_base... sdk/lib/crypto/crypto_base.dart:145: * Based on RFC 4686 http://tools.ietf.org/html/rfc4648 On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > 4686 -> 4648 Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:21: "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/".codeUnits; On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Consider not extracting code units here, and just call 'codeUnitAt(i)' where you > use it. The codeUnits object is just a wrapper around the string, and it does > exactly the same thing, just with more overhead. > > Do add a type to the declaration (String or List<int> depending on the above). Converted to String. But I am curious, I thought "codeUnits" would return a copy of strings characters in List<int> form, similar to java's toCharArray method. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:24: "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_".codeUnits; On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Ditto. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:26: static const List<int> _decodeTable = On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Add a comment, it doesn't have to be dartdoc, to explain the > -2 and -1. > I can guess it, but it's still better to not have to guess. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:31: -2, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Consider padding with spaces here so every number has the same size (and yes, it > requires re-breaking every line after here :) Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:44: static String bytesToBase64(List<int> bytes, [bool urlSafe, bool addLineSeparator]) { On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Long line needs to be broken. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:45: int len = bytes != null ? bytes.length : 0; On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Consider not accepting null. It may just be hiding an error. > The caller can check for null if he knows it's possible and just not call at all > in that case. I removed the null check, but not sure how to handle null input. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:51: final int cLen = len ~/ 3 * 3; On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > cLen is a non-obvious abbreviated name. Consider giving it a longer name > (characterLength, chunkCount for just "len ~/ 3" or similar). Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:52: final int padLen = len - cLen; On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > padLength. > > Is this done this way because it's faster than > int padLen = len % 3; > int cLen = len - padLen; > ? > I wouldn't be surprised if it is. Done. So obvious :/ https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:57: oLen += (oLen - 1) ~/ 76 << 1; On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > Parentheses around the left-hand-side of <<. Nobody remembers whether ~/ or << > has higher priority :) Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:79: int x = bytes[cLen] << 10 | (padLen == 2 ? bytes[len - 1] << 2 : 0); On 2013/03/21 14:06:38, Lasse Reichstein Nielsen wrote: > More parentheses! Does << bind harder than | ? Who'd have thunk?! > > There's an extra space after 10. Done.
LGTM with comments addressed (if the counting of extrasLen is correct). https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:21: "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/".codeUnits; It just returns a read-only list view of the code units, backed by the string. Since the string is immutable, it is cheaper (in memory) to just use the string instead of creating a new list. If you really need a new list (e.g., because you want change it), you can call toList on that list. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:44: static String bytesToBase64(List<int> bytes, [bool urlSafe, bool addLineSeparator]) { Default for urlSafe and addLineSeparator is null. It will probably always be called with non-null values, but maybe make the default false in any case. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:45: int len = bytes != null ? bytes.length : 0; That would make bytes.length throw, which is perfectly fine. In general, I don't say anything if a typed parameter does not accept null, and instead document what it means if it does accept null. Throwing the equivalent of a null-pointer-error here will ensure that the null doesn't go any further. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:90: int len = input != null ? input.length : 0; Again, feel free to throw on null. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:95: // Count \r \n and illegal chars, // Count '\r', '\n' and illegal characters. Makes it more obvious that \r and \n are referring to characters. Start with capital letter on next line. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:102: extrasLen++; So if c is -1, we increment extrasLen if we ignoreErrors, but not if we don't, but we don't throw in either case. That doesn't look right. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:105: 'Invalid character ${new String.fromCharCode(input.codeUnitAt(i))}'); ${input[i]} should be equivalent, and shorter, which is good since the line is too long :) https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:110: if ((len - extrasLen) % 4 != 0) { Is this requirement documented anywhere? Is it tested? Both with and without ignore errors :) https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:121: int oLen = ((len - extrasLen) * 6 >> 3) - pad; More parentheses. Anything near a >> needs to be parenthesized :) A more telling name! https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:126: int j = 18; How about: for (int j = 4; j > 0;) { int c = _decodeTable[input.codeUnitAt(i++)]; if (c >= 0) { x = (x << 6) | c; j--; } } It's really the same, but the shift is by a constant and not dependent on the value of c. I.e., I think it may be marginally faster :) https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:139: out[o++] = x & 0xFF; Always use braces around then/else branches for a multi-line if. I.e., only omit them if it's all on one line. https://codereview.chromium.org/12534011/diff/1001/tests/lib/crypto/base64_te... File tests/lib/crypto/base64_test.dart (right): https://codereview.chromium.org/12534011/diff/1001/tests/lib/crypto/base64_te... tests/lib/crypto/base64_test.dart:12: const [ '', 'f', 'fo', 'foo', 'foob', 'fooba', 'foobar' ]; Please add inputs that end in '\0' and '\0\0' too, and check that they roundtrip correctly. https://codereview.chromium.org/12534011/diff/1001/tests/lib/crypto/base64_te... tests/lib/crypto/base64_test.dart:102: testEncodeDecode(); Add tests for url-safe and ignoreErrors too.
LGTM. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_bas... File sdk/lib/crypto/crypto_base.dart (right): https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_bas... sdk/lib/crypto/crypto_base.dart:136: * If [usePadding] is false, Extra padding characters ('=') are not If [usePadding] is false, no extra padding characters ('=') are appended to the output. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_bas... sdk/lib/crypto/crypto_base.dart:161: * Returns an empty list if [input] is null. Why not throw a null-pointer exception? (in which case we shouldn't say anything about `null` in the comment. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:16: static final int PAD = '='.codeUnitAt(0); I would make this a const. Even if that means that we cannot use codeUnitAt. something like: static const int PAD = 100; // '='. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:20: static final String _encodeTable = const, even though it shouldn't matter much. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:23: static final String _encodeTableUrlSafe = ditto. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:51: bool addLineSeparator = false, nit: add one space in front of the "bool addLi..." and the next line. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:65: outputLen += ((outputLen - 1) ~/ 76) << 1; Maybe put 76 in a constant? https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:72: int x = (bytes[i++] << 16) | (bytes[i++] << 8) | bytes[i++]; I wonder if you could help the VM and dart2js if you bit-anded with 24bits. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:85: // If input length if not a multiple of 3, encode remaining add padding. encode the remaining bytes and add padding. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:117: if (c == -1) { indentation. (2chars) https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:119: } else if (c == -2 && !ignoreErrors) { Change this if to: if (c < 0) { extrasLen++; if (c == -2 && !ignoreErrors) { throw new FormatException ...; } } Otherwise the test (len - extrasLen) % 4 != 0 won't work if there are non-new-line characters. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:125: if ((len - extrasLen) % 4 != 0) { Needs change above to work. Add test for it. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:131: for (int i = len; i > 1 && _decodeTable[input.codeUnitAt(--i)] <= 0;) { I prefer: for (int i = len - 1; i >= 0; i--) { int currentCodeUnit = input.codeUnitAt(i); if (_decodeTable[currentCodeUnit] > 0) break; if (currentCodeUnit == PAD) padLength++; } https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:145: x = (x << 6) | c; I would bit-and to help the vm and dart2js. Otherwise they need to verify that the left-shifting won't hurt them. https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... File tests/lib/crypto/base64_test.dart (right): https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... tests/lib/crypto/base64_test.dart:52: Expect.equals(CryptoUtils.bytesToBase64(longLine.codeUnits, addLineSeparator : true), long line. https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... tests/lib/crypto/base64_test.dart:71: CryptoUtils.base64StringToBytes("AB~", ignoreErrors : false); We usually use: Expect.throws(() { CryptoUtils.base64StringToBytes("AB~", ignoreErrors: false); }, (e) => e is FormatException); Note: no space before the ":" of named arguments. https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... tests/lib/crypto/base64_test.dart:121: print("Encode 1024 bytes for $iters times: $ms msec. $perSec b/s"); Don't print in tests. But you can leave it behind comments. https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... tests/lib/crypto/base64_test.dart:128: print("Decode into ${l.length} bytes for $iters times: $ms msec. $perSec b/s"); 80 chars.
I hipe I fixed all the issues, would be nice if you have a quick look, there are some minor open issues as well. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:21: "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/".codeUnits; On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > It just returns a read-only list view of the code units, backed by the string. > Since the string is immutable, it is cheaper (in memory) to just use the string > instead of creating a new list. > If you really need a new list (e.g., because you want change it), you can call > toList on that list. Acknowledged. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:44: static String bytesToBase64(List<int> bytes, [bool urlSafe, bool addLineSeparator]) { On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > Default for urlSafe and addLineSeparator is null. > It will probably always be called with non-null values, but maybe make the > default false in any case. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:45: int len = bytes != null ? bytes.length : 0; On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > That would make bytes.length throw, which is perfectly fine. > In general, I don't say anything if a typed parameter does not accept null, and > instead document what it means if it does accept null. Throwing the equivalent > of a null-pointer-error here will ensure that the null doesn't go any further. Acknowledged. So basically, if it accepts null document it, let it throw if it does not and got a null parameter instead. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:90: int len = input != null ? input.length : 0; On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > Again, feel free to throw on null. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:95: // Count \r \n and illegal chars, On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > // Count '\r', '\n' and illegal characters. > > Makes it more obvious that \r and \n are referring to characters. Start with > capital letter on next line. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:102: extrasLen++; On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > So if c is -1, we increment extrasLen if we ignoreErrors, but not if we don't, > but we don't throw in either case. That doesn't look right. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:105: 'Invalid character ${new String.fromCharCode(input.codeUnitAt(i))}'); On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > ${input[i]} should be equivalent, and shorter, which is good since the line is > too long :) Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:110: if ((len - extrasLen) % 4 != 0) { On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > Is this requirement documented anywhere? > Is it tested? Both with and without ignore errors :) Not really documented, but in theory if you count out the \r \n and illegal characters, remaining should be multiple of four otherwise the inner decoding loop would throw. However there is a case to consider, if there are 6 pad chars instead of 2, it will still be a multiple of 4 with 4 extra pad chars but an invalid input. I can add a check to padLength as well to catch that. Another question is, if ignoreErrors is not specified, why is this throwing an error, maybe we can change the parameter to "ignoreInvalidCharacters" and make its scope narrower. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:121: int oLen = ((len - extrasLen) * 6 >> 3) - pad; On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > More parentheses. Anything near a >> needs to be parenthesized :) > A more telling name! Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:126: int j = 18; On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > How about: > for (int j = 4; j > 0;) { > int c = _decodeTable[input.codeUnitAt(i++)]; > if (c >= 0) { > x = (x << 6) | c; > j--; > } > } > It's really the same, but the shift is by a constant and not dependent on the > value of c. I.e., I think it may be marginally faster :) Done. Much better. Tested it, this version is indeed faster by ~%2-3. thanks. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:139: out[o++] = x & 0xFF; On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > Always use braces around then/else branches for a multi-line if. I.e., only omit > them if it's all on one line. Done. https://codereview.chromium.org/12534011/diff/1001/tests/lib/crypto/base64_te... File tests/lib/crypto/base64_test.dart (right): https://codereview.chromium.org/12534011/diff/1001/tests/lib/crypto/base64_te... tests/lib/crypto/base64_test.dart:12: const [ '', 'f', 'fo', 'foo', 'foob', 'fooba', 'foobar' ]; On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > Please add inputs that end in '\0' and '\0\0' too, and check that they roundtrip > correctly. > I am not sure significance of '\0' , it is interpreted as "0" in dart? I added [0,0,0] and see if its "AAAA". Does this makes any sense? https://codereview.chromium.org/12534011/diff/1001/tests/lib/crypto/base64_te... tests/lib/crypto/base64_test.dart:102: testEncodeDecode(); On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > Add tests for url-safe and ignoreErrors too. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_bas... File sdk/lib/crypto/crypto_base.dart (right): https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_bas... sdk/lib/crypto/crypto_base.dart:136: * If [usePadding] is false, Extra padding characters ('=') are not On 2013/03/22 17:22:05, floitsch wrote: > If [usePadding] is false, no extra padding characters ('=') are appended to the > output. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_bas... sdk/lib/crypto/crypto_base.dart:161: * Returns an empty list if [input] is null. On 2013/03/22 17:22:05, floitsch wrote: > Why not throw a null-pointer exception? (in which case we shouldn't say anything > about `null` in the comment. Done, no mention of null anymore. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:16: static final int PAD = '='.codeUnitAt(0); On 2013/03/22 17:22:05, floitsch wrote: > I would make this a const. Even if that means that we cannot use codeUnitAt. > something like: > static const int PAD = 100; // '='. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:20: static final String _encodeTable = On 2013/03/22 17:22:05, floitsch wrote: > const, even though it shouldn't matter much. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:23: static final String _encodeTableUrlSafe = On 2013/03/22 17:22:05, floitsch wrote: > ditto. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:51: bool addLineSeparator = false, On 2013/03/22 17:22:05, floitsch wrote: > nit: add one space in front of the "bool addLi..." and the next line. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:65: outputLen += ((outputLen - 1) ~/ 76) << 1; On 2013/03/22 17:22:05, floitsch wrote: > Maybe put 76 in a constant? Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:72: int x = (bytes[i++] << 16) | (bytes[i++] << 8) | bytes[i++]; On 2013/03/22 17:22:05, floitsch wrote: > I wonder if you could help the VM and dart2js if you bit-anded with 24bits. Done. I added a 0xFFFFFF mask per << operation, hope that was what you meant. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:85: // If input length if not a multiple of 3, encode remaining add padding. On 2013/03/22 17:22:05, floitsch wrote: > encode the remaining bytes and add padding. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:117: if (c == -1) { On 2013/03/22 17:22:05, floitsch wrote: > indentation. (2chars) Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:117: if (c == -1) { On 2013/03/22 17:22:05, floitsch wrote: > indentation. (2chars) Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:119: } else if (c == -2 && !ignoreErrors) { On 2013/03/22 17:22:05, floitsch wrote: > Change this if to: > if (c < 0) { > extrasLen++; > if (c == -2 && !ignoreErrors) { > throw new FormatException ...; > } > } > > Otherwise the test (len - extrasLen) % 4 != 0 won't work if there are > non-new-line characters. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:125: if ((len - extrasLen) % 4 != 0) { On 2013/03/22 17:22:05, floitsch wrote: > Needs change above to work. > Add test for it. Done. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:131: for (int i = len; i > 1 && _decodeTable[input.codeUnitAt(--i)] <= 0;) { On 2013/03/22 17:22:05, floitsch wrote: > I prefer: > for (int i = len - 1; i >= 0; i--) { > int currentCodeUnit = input.codeUnitAt(i); > if (_decodeTable[currentCodeUnit] > 0) break; > if (currentCodeUnit == PAD) padLength++; > } Done. Cleaner, thanks. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:145: x = (x << 6) | c; On 2013/03/22 17:22:05, floitsch wrote: > I would bit-and to help the vm and dart2js. > Otherwise they need to verify that the left-shifting won't hurt them. Done. https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... File tests/lib/crypto/base64_test.dart (right): https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... tests/lib/crypto/base64_test.dart:52: Expect.equals(CryptoUtils.bytesToBase64(longLine.codeUnits, addLineSeparator : true), On 2013/03/22 17:22:05, floitsch wrote: > long line. Done. https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... tests/lib/crypto/base64_test.dart:71: CryptoUtils.base64StringToBytes("AB~", ignoreErrors : false); On 2013/03/22 17:22:05, floitsch wrote: > We usually use: > Expect.throws(() { CryptoUtils.base64StringToBytes("AB~", ignoreErrors: false); > }, > (e) => e is FormatException); > > Note: no space before the ":" of named arguments. Done. https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... tests/lib/crypto/base64_test.dart:121: print("Encode 1024 bytes for $iters times: $ms msec. $perSec b/s"); On 2013/03/22 17:22:05, floitsch wrote: > Don't print in tests. But you can leave it behind comments. Uh, this was meant to be removed before uploading. but I guess keeping the test here doesn't hurt. It's not slow. Commented out print. https://codereview.chromium.org/12534011/diff/12002/tests/lib/crypto/base64_t... tests/lib/crypto/base64_test.dart:128: print("Decode into ${l.length} bytes for $iters times: $ms msec. $perSec b/s"); On 2013/03/22 17:22:05, floitsch wrote: > 80 chars. Done.
LGTM. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:45: int len = bytes != null ? bytes.length : 0; On 2013/03/25 17:05:02, mdakin1 wrote: > On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > > That would make bytes.length throw, which is perfectly fine. > > In general, I don't say anything if a typed parameter does not accept null, > and > > instead document what it means if it does accept null. Throwing the equivalent > > of a null-pointer-error here will ensure that the null doesn't go any further. > > Acknowledged. So basically, if it accepts null document it, let it throw if it > does not and got a null parameter instead. correct. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:110: if ((len - extrasLen) % 4 != 0) { On 2013/03/25 17:05:02, mdakin1 wrote: > On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > > Is this requirement documented anywhere? > > Is it tested? Both with and without ignore errors :) > > Not really documented, but in theory if you count out the \r \n and illegal > characters, remaining should be multiple of four otherwise the inner decoding > loop would throw. > > However there is a case to consider, if there are 6 pad chars instead of 2, it > will still be a multiple of 4 with 4 extra pad chars but an invalid input. I can > add a check to padLength as well to catch that. > > Another question is, if ignoreErrors is not specified, why is this throwing an > error, maybe we can change the parameter to "ignoreInvalidCharacters" and make > its scope narrower. I would be fine with "ignoreInvalidCharacters". It doesn't seem to be a commonly used flag. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:111: throw new FormatException('Invalid input.'); I wonder if this shouldn't contain the input. In any case it should probably say, why it is invalid. https://codereview.chromium.org/12534011/diff/1001/tests/lib/crypto/base64_te... File tests/lib/crypto/base64_test.dart (right): https://codereview.chromium.org/12534011/diff/1001/tests/lib/crypto/base64_te... tests/lib/crypto/base64_test.dart:12: const [ '', 'f', 'fo', 'foo', 'foob', 'fooba', 'foobar' ]; On 2013/03/25 17:05:02, mdakin1 wrote: > On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > > Please add inputs that end in '\0' and '\0\0' too, and check that they > roundtrip > > correctly. > > > I am not sure significance of '\0' , it is interpreted as "0" in dart? I added > [0,0,0] and see if its "AAAA". Does this makes any sense? I think you could have written '\x00' in the string to avoid having a new variable, but the current version LGTM. https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/12002/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:72: int x = (bytes[i++] << 16) | (bytes[i++] << 8) | bytes[i++]; On 2013/03/25 17:05:02, mdakin1 wrote: > On 2013/03/22 17:22:05, floitsch wrote: > > I wonder if you could help the VM and dart2js if you bit-anded with 24bits. > > Done. > I added a 0xFFFFFF mask per << operation, hope that was what you meant. Yes. that's what I wanted. https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_bas... File sdk/lib/crypto/crypto_base.dart (right): https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_bas... sdk/lib/crypto/crypto_base.dart:139: * Based on RFC 4648 http://tools.ietf.org/html/rfc4648 nit: I think Markdown has a way to specify URLs. Not sure dartdoc supports it, but would be nice if it did. https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_bas... sdk/lib/crypto/crypto_base.dart:159: * Based on RFC 4648 http://tools.ietf.org/html/rfc4648 ditto. https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_uti... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:59: final int remainderLength = len % 3; Since it's called "remainderLength" you could use "remainder": remainderLength = len.remainder(3); In theory it's faster too (although it shouldn't matter). I'm perfectly happy with "%", though! https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:132: for (int i = len -1; i >= 0; i--) { len - 1 (space after "-")
https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:110: if ((len - extrasLen) % 4 != 0) { On 2013/03/25 17:45:37, floitsch wrote: > On 2013/03/25 17:05:02, mdakin1 wrote: > > On 2013/03/22 09:16:56, Lasse Reichstein Nielsen wrote: > > > Is this requirement documented anywhere? > > > Is it tested? Both with and without ignore errors :) > > > > Not really documented, but in theory if you count out the \r \n and illegal > > characters, remaining should be multiple of four otherwise the inner decoding > > loop would throw. > > > > However there is a case to consider, if there are 6 pad chars instead of 2, it > > will still be a multiple of 4 with 4 extra pad chars but an invalid input. I > can > > add a check to padLength as well to catch that. > > > > Another question is, if ignoreErrors is not specified, why is this throwing an > > error, maybe we can change the parameter to "ignoreInvalidCharacters" and make > > its scope narrower. > > I would be fine with "ignoreInvalidCharacters". It doesn't seem to be a commonly > used flag. Done. https://codereview.chromium.org/12534011/diff/1001/sdk/lib/crypto/crypto_util... sdk/lib/crypto/crypto_utils.dart:111: throw new FormatException('Invalid input.'); On 2013/03/25 17:45:37, floitsch wrote: > I wonder if this shouldn't contain the input. > In any case it should probably say, why it is invalid. Added input to error, explanation is a little bit dodgy but probably ok for now. https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_bas... File sdk/lib/crypto/crypto_base.dart (right): https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_bas... sdk/lib/crypto/crypto_base.dart:139: * Based on RFC 4648 http://tools.ietf.org/html/rfc4648 On 2013/03/25 17:45:37, floitsch wrote: > nit: I think Markdown has a way to specify URLs. Not sure dartdoc supports it, > but would be nice if it did. Done. https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_bas... sdk/lib/crypto/crypto_base.dart:159: * Based on RFC 4648 http://tools.ietf.org/html/rfc4648 On 2013/03/25 17:45:37, floitsch wrote: > ditto. Done. https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_uti... File sdk/lib/crypto/crypto_utils.dart (right): https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:59: final int remainderLength = len % 3; On 2013/03/25 17:45:37, floitsch wrote: > Since it's called "remainderLength" you could use "remainder": > remainderLength = len.remainder(3); > In theory it's faster too (although it shouldn't matter). > I'm perfectly happy with "%", though! Done. I agree, remainder is more readable. https://codereview.chromium.org/12534011/diff/17001/sdk/lib/crypto/crypto_uti... sdk/lib/crypto/crypto_utils.dart:132: for (int i = len -1; i >= 0; i--) { On 2013/03/25 17:45:37, floitsch wrote: > len - 1 (space after "-") Done.
Message was sent while issue was closed.
Committed patchset #9 manually as r20845 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
