On 10/17/18, 3:03 PM, Stuart Marks wrote:
Hi Joe,
webrevs: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v05/
I concur with Roger's comments on the test file generation. It would
be good to find some way to reduce the redundancy. He had some
suggestions that might work. You might also want to play around with
variations to see how they look.
375 byte[] bytes = "ABCDEFGHIJKLMNOPQRSTUVWXYZ
abcdefghijklmnopqrstuvwxyz 0123456789 \n".getBytes();
A small thing, but this uses the default charset, which is
platform-specific. I think it would be good to use
StandardCharsets.US_ASCII here.
ASCII it is.
Once you have this in bytes, it looks like all the other parts of the
test deal with bytes (not characters) so I don't think there are any
other concerns about charsets and such.
345 /**
346 * Creates a file with alphanumeric content with one
character altered
347 * at the specified position.
348 *
349 * @param dir the directory in which the file is to be created
350 * @param purpose the purpose of the file
351 * @param size the size of the file
352 * @param pos the position where the alternative char is to
be added. If it
353 * is smaller than zero, no alternation shall be
made.
354 * @param c the character
355 * @return path of the created file
356 * @throws IOException
357 */
358 private static Path createANFile(Path dir, String purpose,
int size, int pos,
359 byte c) throws IOException {
360 Path path = Files.createFile(Paths.get(dir.toString(),
purpose + ".txt"));
361 if (size > 0) {
362 writeAlphanumericFile(path, size, pos, c);
363 }
364 return path;
365 }
The problem with documenting things well is that the doc gets out of
date. :-)
Very true, now I get to keep the doc as we switch back to char :-)
(Well, actually somewhat serious.)
This method creates test data files in terms of bytes (although the
bytes happen to be derived from ASCII characters). The altered data is
a byte at a particular byte offset, not a character. Several places in
the doc need to be updated to reflect this, since they still talk
about characters.
Even though the altered data is a byte, for the convenience of the
caller, the parameter should be a char or an int. All the places where
this is called use a char literal casted to a byte:
125 test1024m1014a = createANFile(testDir, "test1024m1014a",
size, size - 10, (byte)'@');
If the parameter were a char or a int, the caller could just do:
125 test1024m1014a = createANFile(testDir, "test1024m1014a",
size, size - 10, '@');
The byte used to alter the test data could be the low order 7 bits
(since we're using ASCII) of the parameter value. Either document it
this way and simply mask off the high order bits of the parameter
value, or assert that those high order bits are all zeroes.
Sounds good, the latter looks better with a char. It's a plus as I can
keep the statement that says it alters one character. While only the low
order byte matters for this test data (ASCII only), it's masked off to
be clear about the intention.
In writeAlphanumericFile(),
370 if (pos > 0) a[pos] = c;
This should check should be for pos >= 0, since altering a byte at
position 0 is a valid test case. You might want to add this as another
test case.
Changed to >=0, and added test cases for an altered byte at position 0
(as well as at the end).
Current version:
http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v06/
Previous:
http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev_v05/
Thanks,
Joe
s'marks
