> Continuing this review as a PR on github with the comments below > incorporated. I expect there will be a few more iterations before integrating. > > On 06/09/2020 19:47, Alan Bateman wrote: >> On 26/08/2020 15:24, Michael McMahon wrote: >>> >>> As I mentioned the other day, I wasn't able to use the suggested method on >>> Windows which returns an absolute path. So, I added a method to WindowsPath >>> which returns the path in the expected UTF-8 encoding as a byte[]. Let me >>> know what you think of that. >>> >>> There is a fair bit of other refactoring and simplification done also. Next >>> version is at: >>> >>> http://cr.openjdk.java.net/~michaelm/8245194/impl.webrev/webrev.9/ >>> >> Adding a method to WindowsPath to encode the path using UTF-8 is okay but I >> don't think we should be caching it as the encoding for sun_path is an >> outlier on Windows. I'm also a bit dubious about encoding a relative path >> when the resolved path (before encoding) is > 247 chars. The documentation >> on the MS site isn't very completely and I think there are a number points >> that require clarification to fully understand how this will work with >> relative, directly relative and drive relative paths. >> > > Maybe it would be better to just use the path returned from toString() and do > the conversion to UTF-8 externally. That would leave WindowsPath unchanged. > >> In the same area, the new PathUtil is a bit inconsistent with the existing >> provider code. One suggestion is to add a method to >> AbstractFileSystemProvider instead. That is the base class the platform >> providers and would be a better place to get the file path in bytes. >> > > Okay, I gave the method a name that is specific to Unix domain sockets > because this UTF-8 strangeness is not likely to be useful by other components. > >> One other comment on the changes to the file system provider it should be >> okay to change UnixUserPrinipals ad its fromUid and fromGid method to be >> public. This would mean that the patch shouldn't need to add UnixUserGroup >> (the main issue is that class is that it means we end up with two classes >> with static factory methods doing the same thing). > > Okay, that does simplify it a bit. > > Thanks, > Michael. > >> -Alan. >> >> >> >> >> >>
Michael McMahon has updated the pull request incrementally with one additional commit since the last revision: updated one error in review ------------- Changes: - all: https://git.openjdk.java.net/jdk/pull/52/files - new: https://git.openjdk.java.net/jdk/pull/52/files/e07522b5..368fa8bc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=32 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=52&range=31-32 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/52.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/52/head:pull/52 PR: https://git.openjdk.java.net/jdk/pull/52
