On 9/20/18, 2:46 PM, Stuart Marks wrote:
On 9/19/18 11:48 AM, Joe Wang wrote:
After much discussion and 10 iterations of reviews, this proposal has
evolved from what was the original isSameContent method to a mismatch
method. API-wise, a compare method was also considered as it looked
like just a short step forward from mismatch, however, it was
eventually dropped since there is no convincing use case comparing
files lexicographically by contents. Impl-wise, extensive performance
benchmarking has been done to compare a buffered reading vs memory
mapping, the result was that a simple buffered reading performed
better among small files, and those with the mismatched byte closer
to the beginning of files. Since the proposed method's targeted files
are small ones, the impl currently does a buffered reading only.
Hi Joe,
Thanks for being persistent with this one!
Thanks for the help, and the "lot of stuff" with clear indicators (e.g.
line numbers and etc.)! From JDK 11 to 12, hopefully wont be deferred
to 13 :-)
A very small spec nitpick:
specdiff:
http://cr.openjdk.java.net/~joehw/jdk12/8202285/specdiff/java/nio/file/Files.html
1544 * <li> {@link #isSameFile(Path, Path) isSameFile(path,
path2)} returns true,</li>
I would add "or" after the trailing comma. This makes it similar to
the two-item list that follows.
Done.
webrev: http://cr.openjdk.java.net/~joehw/jdk12/8202285/webrev/
A couple minor comments on FilesMismatch.java:
If mismatchByAttrs() is replaced with a call to isSameFile(), as Alan
suggested, this simplifies things considerably. It looks like the
mismatch() implementation will be reduced to around ~40 lines of code.
Does it deserve its own file, or can it be placed directly into
Files.java? That file has over 4,000 lines already though.
I merged the private methods after removing mismatchByAttrs(), that
reduced the code to 33 lines, and then with the change you suggested
below, it's further reduced to 29 lines. I'll put them in Files for now.
Files is too big, ~180K, that compares with the next one ~45K among
files in the nio/file package. Unfortunately, the great majority of them
are specs, probably 90%.
106 if (nRead1 == 0 || nRead2 == 0) {
107 if (nRead1 == 0 && nRead2 == 0) {
108 // both files reach EOF
109 return -1;
110 } else {
111 // one of the files reaches EOF
112 return totalRead;
113 }
114 } else if (nRead1 != nRead2) {
I think it reads slightly better if the nested 'if' at lines 107-113
were flattened into the else-if chain, like so:
if (nRead1 == 0 && nRead2 == 0) {
// both files reached EOF
return -1;
} else if (nRead1 == 0 || nRead2 == 0) {
// one but not both files reached EOF
return totalRead;
} else if (nRead1 != nRead2) {
There are a couple places where lines can be joined. The resulting
lines are longer than 80 characters but are still less than 100
characters, which I think is OK. (I don't think we have a strict limit
of 80 anymore.)
97 private static long mismatch(InputStream in1, InputStream in2)
98 throws IOException {
117 return totalRead +
118 Arrays.mismatch(buffer1, 0, len, buffer2, 0,
len);
Done. 95 characters :-)
Comments on tests in Comparison.java:
* This file, and several names within this file, should probably
should be renamed to focus on mismatch instead of comparison.
Will rename to "mismatch". Previously, it covers both mismatch and compare.
* I'm finding it quite difficult to read the test cases.
The test files are placed into a Path[] and referenced by index. This
makes it quite difficult to determine, for example, if all six cases
within the mismatch loop are covered.
Instead of storing the files at specific array indexes, I'd suggest
doing the following.
1) Establish good naming conventions for test files, including size
information, covering duplicate files (e.g., 'a' and 'b'), and a
modification at a particular offset. For example, you might do
something like the following:
test120a - a test file with size 120
test120b - a test file with size 120, identical to test120a
test120m110 - a test file with size 120, with an altered byte at 110
etc.
I can do that. I had hoped the Assertion msg, e.g. "Both files are
empty, no mismatch" would help. Along with renaming variables, I will
also add to the Assertion Msg with test case number, e.g. "Case#1: both
files are empty, return no mismatch".
2) Declare fields of the same name containing the file paths.
3) Add some logic in prepareTestFile() to append each file's path to a
list after the file is created. Then have the @AfterClass method run
through this list and delete the files.
May also create the test files in a folder and delete it @AfterClass.
4) The test data generator method can then have lines of test data
that reference the test files using a meaningful name, so it's easy to
understand the names and the expected results. Also, the test data
generator should have a comment that describes the test method
parameters.
Sure, will add comments and etc.
* The generation of the test data files seems overly complicated.
A StringBuilder is used to accumulate data, then it's turned into a
String, then (optionally) a character is replaced by the insertChar()
method, and then the string is written to the file.
I think you can just accumulate the test data into a StringBuilder,
(optionally) call sb.setCharAt() to modify the data, and then write
the SB to the file. After all, Files.writeString() takes a CharSequence.
The generateFixedString() method should be changed to create and
return a fresh StringBuilder with the desired contents. (See above.)
It can also be simplified quite a bit.
Wouldn't it be nice if String has an utility method that generates
random ASCII strings? ;-) It can be faster with its internal byte array.
I don't think you need to have methods with Charset parameters. It's
reasonable to have the test files be text, which makes things easier
to develop and debug. But the test depends on characters being written
in a single-byte charset, because mismatch() reports *byte* positions
and not *character* positions.
Using the method overloads that are implicitly UTF-8 is incorrect. If
a non-single-byte character were to slip into the test data (seems
unlikely, but possible), it'll be expanded to multiple bytes by the
UTF-8 encoder, which may throw off the result. Instead I'd suggest
hardwiring the charset to be US_ASCII to enforce the
one-byte-per-character invariant.
This is a lot of stuff. If you can't follow it all, perhaps we can
work together directly and go over the test file.
I'll improve the test, and US_ASCII it is! I'll probably use a string of
fixed length to generate the longest string the test needs, and then get
substrings for other test cases, that may simplify the process.
Thanks,
Joe
Thanks,
s'marks
