RFR(JDK 12/NIO) 8202285: (fs) Add a method to Files for comparing file contents

[Joe Wang]

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