Hi Pavel,
On 4/12/2016 10:36 AM, Pavel Rappo wrote:
Hi Roger, thanks for looking into this!
Changes are done in-place.
It saves hunting around if you provide the link.
http://cr.openjdk.java.net/~prappo/8153353/webrev.00/
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 }
Sure, there is nothing clever about the test library version.
If you can't import the test library version, then copy/paste the code
so you get all of its
functions including being able to set the seed via a system property on
the command line.
Since you have a TestHelper class you could put it there and not
duplicate the code in several tests.
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.
it could be as simple as referring to "RFC 7541 section 5.1 Integer
Representation."
- 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.
ok
I've also converted JUnit tests into TestNG ones, as it's de facto standard in
core-libs tests.
good
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;
You don't believe the expression is equivalent?
482 return (len + 7) / 8;
483 }
:)
Thanks, Roger
