Hi Alan, Thanks for reviewing this patch.
On 02/04/2019 11:22, Alan Bateman wrote: > On 02/04/2019 10:12, Andrew Dinn wrote: >> Could I please have reviews for the following enhancement: >> >> JIRA: https://bugs.openjdk.java.net/browse/JDK-8221397 >> webrev: http://cr.openjdk.java.net/~adinn/8221397/webrev.00/ Updated webrev: http://cr.openjdk.java.net/~adinn/8221397/webrev.01 Testing: Updated test (MapTest) passes. Submit job in progress. Details of changes below: > This updates the description for > NonReadableChannelException/NonWriteableChannelException, I assume you > didn't mean to include that in the patch. Yes, that was a leftover from the full JEP change set that I have now removed. > A corner case to consider is whether a map mode can be the empty string, > maybe IAE should be thrown for that case. I have added a test for this case which throws IAE if an empty string is found. I also updated the javadoc accordingly. > Are you planning a test to go along with this? It can test that the map > method throws UOE and it can test the MapMode constructor. I added test code to existing test MapTest as it seemed closely related and was even able to reuse the check method. The new tests ensure that 1) illegal user map mode names are rejected and 2) attempts to pass a user-defined map mode in a map call for a normal file channel are rejected. In order to get this to pass I updated method FileChannelImpl.map to allow for the possibility of user-defined map modes. It no longer asserts if map mode is unrecognized but instead throws an UnsupportedOperationException. I still have two undecided points you might advise on: Does the javadoc for FileChannel.map and/or FileChannelImpl.map need updating to record the possibility that UOE might be thrown? Do I need to update any other implementations of map to cater for the possibility of user-defined map modes? regards, Andrew Dinn ----------- Senior Principal Software Engineer Red Hat UK Ltd Registered in England and Wales under Company Registration No. 03798903 Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander
