On Fri, Oct 29, 2010 at 6:27 PM, Masahiro Nakagawa <repeatedly at gmail.com> wrote:
>
> You are right. Thanks!
>
> http://bitbucket.org/repeatedly/scrap/changeset/70f0d0f16ad1

Great!
I think it should be "'\\0'..." rather than "'\0'...", otherwise it
will just embed a null character into the error.

There should also be another space:
static assert(Padding < '0' || Padding > '9', "Character '" ~ Padding
~ "'cannot be used twice");
static assert(Padding < '0' || Padding > '9', "Character '" ~ Padding
~ "' cannot be used twice");


>
> Your suggestion increases struct size and decreases performance. But, I agree.
>
> http://bitbucket.org/repeatedly/scrap/changeset/75ee04cdd87b
>
It's not very pretty, but without it encoder and decoder don't correctly implement the range interface.

Some more things:
1. isArray should be isDynamicArray
2. Need to import std.array for put(a, b)

3. The 4 implementations of encode could be merged into 2 using static ifs.  This will result in some very ugly code, but will cut down on code duplication and greatly simplify the method signature.  Same thing for decode.

4. The decoder chunk range takes both ubyte[] and char[], while the decode function only takes ubyte[].  They should both probably take the same types.

5. Chunk version of decoder does not call doDecoding in the constructor.

6. Chunk versions of encoder/decoder do not define save correctly (should make a copy of the buffered data to prevent aliasing)

7. The requirement that decoder should take chunks with a size that is a multiple of 4 must either be asserted or removed.  The encoder range needs a warning that padding will be added to the end of each chunk. The warnings need to be much bigger and appear earlier in the documentation for each range.

7.5.  Considering that it says chunks must be a multiple of 4, it is not necessary for doDecoding to read more than one chunk from the input.  This should eliminate all the extra allocations in Decoder.

8. The encoder and decoder shortcut functions probably need to have a copy of the template constraints.  This will give better error messages.

9. The line Appender!(char[])([]); gives me an error about converting
arrays of void[].  Changing it to Appender!(char[])(null); gets rid of
it.  Is this still present with the newest dmd?

10. The block of unittests at 1450 still refuses to compile for me. Again, this might be because I'm using 2.048.  The errors are about using to!ubyte, and passing map to encode.

11.  Some of the documentation comments are not very good english (sorry).  I assume you are not a native english speaker, I can update them if you would like.

Thanks!

11. Please give me a diff file.

I cannot use a Mac, so I will reply other things later.

Masahiro

2010/10/29 20:03 "Daniel Murphy" <yebblies at gmail.com>:

On Fri, Oct 29, 2010 at 6:27 PM, Masahiro Nakagawa <repeatedly at gmail.com> wrote:
>
> You are right. ...
Great!
I think it should be "'\\0'..." rather than "'\0'...", otherwise it
will just embed a null character into the error.

There should also be another space:
static assert(Padding < '0' || Padding > '9', "Character '" ~ Padding
~ "'cannot be used twice");
static assert(Padding < '0' || Padding > '9', "Character '" ~ Padding
~ "' cannot be used twice");



>
> Your suggestion increases struct size and decreases performance. But, I agree.
>
> http://bi...
It's not very pretty, but without it encoder and decoder don't correctly implement the range interface.

Some more things:
1. isArray should be isDynamicArray
2. Need to import std.array for put(a, b)

3. The 4 implementations of encode could be merged into 2 using static ifs.  This will result in some very ugly code, but will cut down on code duplication and greatly simplify the method signature.  Same thing for decode.

4. The decoder chunk range takes both ubyte[] and char[], while the decode function only takes ubyte[].  They should both probably take the same types.

5. Chunk version of decoder does not call doDecoding in the constructor.

6. Chunk versions of encoder/decoder do not define save correctly (should make a copy of the buffered data to prevent aliasing)

7. The requirement that decoder should take chunks with a size that is a multiple of 4 must either be asserted or removed.  The encoder range needs a warning that padding will be added to the end of each chunk. The warnings need to be much bigger and appear earlier in the documentation for each range.

7.5.  Considering that it says chunks must be a multiple of 4, it is not necessary for doDecoding to read more than one chunk from the input.  This should eliminate all the extra allocations in Decoder.

8. The encoder and decoder shortcut functions probably need to have a copy of the template constraints.  This will give better error messages.

9. The line Appender!(char[])([]); gives me an error about converting
arrays of void[].  Changing it to Appender!(char[])(null); gets rid of
it.  Is this still present with the newest dmd?

10. The block of unittests at 1450 still refuses to compile for me. Again, this might be because I'm using 2.048.  The errors are about using to!ubyte, and passing map to encode.

11.  Some of the documentation comments are not very good english (sorry).  I assume you are not a native english speaker, I can update them if you would like.

Thanks!

_______________________________________________
phobos mailing list
phobos at puremagic.com
http://list...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.puremagic.com/pipermail/phobos/attachments/20101030/dad372f1/attachment.html>

On Fri, 29 Oct 2010 20:02:10 +0900, Daniel Murphy <yebblies at gmail.com> wrote:

> Some more things:
> 1. isArray should be isDynamicArray

Why? I think following code should work.

     ubyte[4] arr = [1, 2, 3, 4];
     Base64.encode(arr);

> 2. Need to import std.array for put(a, b)

No. std.range imports std.array as public.

> 3. The 4 implementations of encode could be merged into 2 using static ifs.  This will result in some very ugly code, but will cut down on code duplication and greatly simplify the method signature.  Same thing for decode.

I don't think so. We have constraint for type separating.

> 4. The decoder chunk range takes both ubyte[] and char[], while the decode function only takes ubyte[].  They should both probably take the same types.

This is sensitive problem. encode method performs Base64 encoding, but
Encoder itself doesn't.
Encoder is a convenient Range wrapper, so I loose the type limit.

> 5. Chunk version of decoder does not call doDecoding in the constructor.

I noticed this. I removed doDecoding for actual code and forgot to add doDecoding.

> 6. Chunk versions of encoder/decoder do not define save correctly (should make a copy of the buffered data to prevent aliasing)

Oops.

> 7. The requirement that decoder should take chunks with a size that is a multiple of 4 must either be asserted or removed.  The encoder range needs a warning that padding will be added to the end of each chunk. The warnings need to be much bigger and appear earlier in the documentation for each range.
>
> 7.5.  Considering that it says chunks must be a multiple of 4, it is not necessary for doDecoding to read more than one chunk from the input.  This should eliminate all the extra allocations in Decoder.

I don't understand this point.
ByChunk doesn't concern Encoder.

> 8. The encoder and decoder shortcut functions probably need to have a copy of the template constraints.  This will give better error messages.

You are right. I will add common constraint.

> 9. The line Appender!(char[])([]); gives me an error about converting
> arrays of void[].  Changing it to Appender!(char[])(null); gets rid of
> it.  Is this still present with the newest dmd?
>
> 10. The block of unittests at 1450 still refuses to compile for me. Again, this might be because I'm using 2.048.  The errors are about using to!ubyte, and passing map to encode.

I don't know the legacy dmd.


Masahiro