Hi Roger, thanks for looking into this!
Changes are done in-place.
Do you think issues marked as [*] could be addressed incrementally after the
initial push?
> On 11 Apr 2016, at 16:18, Roger Riggs <roger.ri...@oracle.com> wrote:
>
> Hi Pavel,
>
> Though this is an implementation only package, it could use a few more
> comments
> to support maintainers. In the package.html, a couple of hints about what
> classes are
> the external interface would be useful and an example of use in Encode and/or
> Decode
> would help a lot.
[*]
> The tests use randomness and should have @key randomness in the tests.
>
> To allow reproducibility, the tests should print the seeds used and be able
> to be restarted
> with a seed. The testlibrary has a convenient factory for Random that logs
> the seed.
>
> See jdk.testlibrary.RandomFactory
>
> * @library /lib/testlibrary/
> * @build jdk.testlibrary.RandomFactory
>
> and use it as:
> private static final Random random = RandomFactory.getRandom();
Since these tests are "white box", I find it hard to inject
jdk.testlibrary.RandomFactory into them, given they are compiled-in
java.httpclient module.
Would it be an acceptable work around to manually log-and-set seeds for any PRNG
used in tests?
113 private final Random rnd;
114 {
115 long seed = System.currentTimeMillis();
116 System.out.println(seed);
117 rnd = new Random(seed);
118 }
> IntegerReader:
> - It would be useful to have a reference to the spec for variable length
> integers: i.e. rfc 7541 5.1 Integer Representation
[*] I have it in my TODO list.
> - Line 129, 53: inconsistent IAE's in some the valid condition is included in
> the exception message,
> but in others the *in*valid condition is included but without any
> indication of which is which.
>
> - line 109/110: style put 'while' on same line as '}' from do so it does not
> look like a separate statement.
>
> IntegerWriter:
> - line 40: perhaps 'value' or 'v' would be more readable than 'i' which is
> often thought of as a local value.
Done.
I've also converted JUnit tests into TestNG ones, as it's de facto standard in
core-libs tests. Fixed remainder operator '%' mentioned by Simone, which I guess
now reads better (and explained the intention as an assert):
474 public int lengthOf(CharSequence value, int start, int end) {
475 int len = 0;
476 for (int i = start; i < end; i++) {
477 char c = value.charAt(i);
478 len += INSTANCE.codeOf(c).length;
479 }
480 // Integer division with ceiling, assumption:
481 assert (len / 8 + (len % 8 != 0 ? 1 : 0)) == (len + 7) / 8 : len;
482 return (len + 7) / 8;
483 }
