Ok, I went through the entire change. Overall it looks pretty good. I hope
that performance of the text format is indeed not considerably worse. And
thank you very much for adding all the tests!



On Mon, Jan 13, 2014 at 2:48 PM, Mikhail Dmitriev <countmdm@gmail.com>wrote:

> Hi Benjy,
>
> I am still looking at this code. Didn't think you get these comments as
> soon as I write them (I am used to the "review-publish" model) - but,
> anyway, thank you for fixing this so quickly. I guess I am going to have
> some more comments. Will let you know when I am done.
>
>
>
> On Mon, Jan 13, 2014 at 2:42 PM, Benjy Weinberger <
> notifications@github.com> wrote:
>
>> In src/com/sun/tools/jmake/Base64.java:
>>
>> > +
>> > +import java.util.Arrays;
>> > +
>> > +
>> > +// JMake needs to run against old versions of Java, that may not have JAXB's
>> > +// javax.xml.bind.DatatypeConverter. And we don't want JMake to depend on third-party external libraries,
>> > +// especially not just for this.  So we implement a lightweight Base64 converter here ourselves.
>> > +//
>> > +// Note that sun.misc.BASE64Encoder is not official API and can go away at any time. Plus it inserts
>> > +// line breaks into its emitted string, which is not what we want. So we can't use that either.
>> > +
>> > +// The easiest way to grok this code is to think of Base64 as the following chain of
>> > +// conversions (ignoring padding issues):
>> > +// 3 bytes -> 24 bits -> 4 6-bit nibbles -> 4 indexes from 0-63 -> 4 characters.
>> > +public class Base64 {
>> > +    private static char[] indexToDigit = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/".toCharArray();
>>
>> Fixed both. Want a pull-request, or shall I just push directly to master,
>> since they are tiny changes?
>>
>> —
>> Reply to this email directly or view it on GitHub<https://github.com/pantsbuild/jmake/pull/2/files#r8843863>
>> .
>>
>
>