On Fri, 14 Jul 2023 12:06:29 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
> Please review this PR to use modern APIs and language features to simplify
> equals, hashCode, and compareTo for in java.nio and implementation code.
>
> Please note, test results are pending.
>
> Additional notes:
>
> * This PR saves a volatile read in java.nio.file.attribute.AclEntry.hashCode.
> Not that it's too important, but worth noting because of rearrangements.
>
> * java.nio.charset.Charset#compareTo seems **inconsistent** with equals. If
> so, I cannot see where that inconsistency is specified.
>
> * Is this a **bug** in sun.nio.ch.FileKey#hashCode? Tell me if not, I'll
> revert it.
>
> * This PR simplifies the tail of java.nio.file.attribute.FileTime.compareTo.
> Unless I'm missing something, that comment in source above the affected lines
> **seems** not to prohibit such a simplification.
>
> * sun.nio.fs.UnixFileStore#hashCode does not include entry.name(). While it's
> not wrong, I wonder if it was on purpose.
>
> * Despite its title, this PR also and opportunistically refactors
> sun.nio.fs.UnixPath.endsWith.
src/java.base/share/classes/java/nio/charset/Charset.java line 957:
> 955: * is less than, equal to, or greater than the specified
> charset
> 956: */
> 957: @Override
This PR adds `@Override` to a subset of the overridden methods, probably best
to add it to all overridden methods so we don't end up with a mix.
src/java.base/share/classes/java/nio/charset/Charset.java line 963:
> 961:
> 962: /**
> 963: * {@return a hashcode for this charset}
If you changing this then probably better to use "the hashcode".
src/java.base/share/classes/java/nio/file/attribute/FileTime.java line 368:
> 366: return cmp;
> 367: }
> 368: return Long.compare(toExcessNanos(days),
> other.toExcessNanos(daysOther));
Maybe a coin toss, but I think the original code was a bit clearer.
src/java.base/unix/classes/sun/nio/fs/UnixFileKey.java line 53:
> 51: if (!(obj instanceof UnixFileKey other))
> 52: return false;
> 53: return (this.st_dev == other.st_dev) && (this.st_ino ==
> other.st_ino);
Style-wise, I think usages like this are a bit easier to read.
return obj instanceof UnixFileKey other
&& (this.st_dev == other.st_dev)
&& (this.st_ino == other.st_ino);
src/java.base/unix/classes/sun/nio/fs/UnixPath.java line 712:
> 710: int thatPos = that.offsets[0];
> 711: return Arrays.equals(this.path, thisPos, thisLen, that.path,
> thatPos,
> 712: thatLen);
Might be cleaner to not put a line break here.
src/java.base/unix/classes/sun/nio/fs/UnixPath.java line 731:
> 729: if (h == 0) {
> 730: h = ArraysSupport.vectorizedHashCode(path, 0, path.length, 0,
> 731: ArraysSupport.T_BOOLEAN);
Can you make this `/* unsigned bytes */ ArraysSupport.T_BOOLEAN`, or
`ArraysSupport.T_BOOLEAN); // unsigned bytes`, only because T_BOOLEAN isn't
always obvious as use-sites.
src/java.base/windows/classes/sun/nio/ch/FileKey.java line 52:
> 50: return (int)(dwVolumeSerialNumber ^ (dwVolumeSerialNumber >>>
> 32)) +
> 51: (int)(nFileIndexHigh ^ (nFileIndexHigh >>> 32)) +
> 52: (int)(nFileIndexLow ^ (nFileIndexLow >>> 32));
Well spotted, probably a cut and paste error at some point, but benign, the
resulting hash code is still okay.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14886#discussion_r1263667696
PR Review Comment: https://git.openjdk.org/jdk/pull/14886#discussion_r1263666839
PR Review Comment: https://git.openjdk.org/jdk/pull/14886#discussion_r1263677841
PR Review Comment: https://git.openjdk.org/jdk/pull/14886#discussion_r1263671563
PR Review Comment: https://git.openjdk.org/jdk/pull/14886#discussion_r1263679533
PR Review Comment: https://git.openjdk.org/jdk/pull/14886#discussion_r1263684952
PR Review Comment: https://git.openjdk.org/jdk/pull/14886#discussion_r1263680738
