Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Thanks all! Glad to be able to get this done right before the deadline :-) -Joe On 6/27/18, 12:44 AM, Alan Bateman wrote: On 26/06/2018 20:49, Joe Wang wrote: : Removed the null check. The internal impl and test guarantees it's not null indeed: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev04/ Looks good. -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 26/06/2018 20:49, Joe Wang wrote: : Removed the null check. The internal impl and test guarantees it's not null indeed: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev04/ Looks good. -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
+1 On 6/26/18, 12:49 PM, Joe Wang wrote: On 6/26/18, 11:57 AM, Alan Bateman wrote: On 26/06/2018 18:41, Joe Wang wrote: : Yes, combined with Sherman's suggestion eliminated the need for the new parameter. Here's the updated webrev: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev03/ This looks good. Just a minor nit but newStringNoRepl and getBytesNoReply don't need to check if the cause is null. Removed the null check. The internal impl and test guarantees it's not null indeed: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev04/ Thanks, Joe -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 6/26/18, 11:57 AM, Alan Bateman wrote: On 26/06/2018 18:41, Joe Wang wrote: : Yes, combined with Sherman's suggestion eliminated the need for the new parameter. Here's the updated webrev: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev03/ This looks good. Just a minor nit but newStringNoRepl and getBytesNoReply don't need to check if the cause is null. Removed the null check. The internal impl and test guarantees it's not null indeed: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev04/ Thanks, Joe -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Thanks Lance! Indeed it has been changed to CCE. I'm still
familiarizing myself with IntelliJ. Opened it in NetBeans, it clearly
indicates "missing @throws tag for " CCE, but I don't see anything like
that in IntelliJ.
Updated webrev:
http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev03/
-Joe
On 6/26/18, 11:24 AM, Lance Andersen wrote:
Hi Joe,
Should src/java.base/share/classes/jdk/internal/misc/JavaLangAccess.java
—
/**
* Constructs a new {@code String} by decoding the specified subarray of
* bytes using the specified {@linkplain java.nio.charset.Charset charset}.
*
* The caller of this method shall relinquish and transfer the ownership of
* the byte array to the callee since the later will not make a copy.
*
* @param bytes the byte array source
* @param cs the Charset
* @return the newly created string
* @throws IllegalArgumentException for malformed or unmappable bytes
*/
String newStringNoRepl(byte[] bytes, Charset cs) throws
CharacterCodingException;
include an @throws for CharacterCodingException?
On Jun 26, 2018, at 1:41 PM, Joe Wang > wrote:
On 6/26/18, 6:54 AM, Alan Bateman wrote:
On 26/06/2018 05:50, Joe Wang wrote:
Hi Alan, Sherman,
Here's a version where we, as Sherman suggested, throw an IAE with
CCE as the cause. This approach reduces code duplication in SC,
although it complicates the impl a little bit with the added
parameter and the different behavior between the existing usages of
the methods and the new ones. The existing code paths are kept
intact so there's no compatibility issue for the existing code.
This version also did not remove the try-catch in Files as Alan
suggested earlier.
http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev02/
Lance
Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 26/06/2018 18:41, Joe Wang wrote: : Yes, combined with Sherman's suggestion eliminated the need for the new parameter. Here's the updated webrev: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev03/ This looks good. Just a minor nit but newStringNoRepl and getBytesNoReply don't need to check if the cause is null. -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Joe,
Should src/java.base/share/classes/jdk/internal/misc/JavaLangAccess.java
—
/**
* Constructs a new {@code String} by decoding the specified subarray of
* bytes using the specified {@linkplain java.nio.charset.Charset charset}.
*
* The caller of this method shall relinquish and transfer the ownership of
* the byte array to the callee since the later will not make a copy.
*
* @param bytes the byte array source
* @param cs the Charset
* @return the newly created string
* @throws IllegalArgumentException for malformed or unmappable bytes
*/
String newStringNoRepl(byte[] bytes, Charset cs) throws
CharacterCodingException;
include an @throws for CharacterCodingException?
> On Jun 26, 2018, at 1:41 PM, Joe Wang wrote:
>
>
>
> On 6/26/18, 6:54 AM, Alan Bateman wrote:
>> On 26/06/2018 05:50, Joe Wang wrote:
>>> Hi Alan, Sherman,
>>>
>>> Here's a version where we, as Sherman suggested, throw an IAE with CCE as
>>> the cause. This approach reduces code duplication in SC, although it
>>> complicates the impl a little bit with the added parameter and the
>>> different behavior between the existing usages of the methods and the new
>>> ones. The existing code paths are kept intact so there's no compatibility
>>> issue for the existing code.
>>>
>>> This version also did not remove the try-catch in Files as Alan suggested
>>> earlier.
>>>
>>> http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev02/
>> This version looks much better. In StringCoding, do you really need
>> throwCCE? The encode/decode methods do a replace or throw so I assume one
>> flag will do. If combined with Sherman suggestion then it would be minimal
>> changes to StringCoding. It would be nice to get rid of the IAE completely
>> but that is for another day. In Files then you don't need to check if cause
>> is null before testing its type.
>
> Yes, combined with Sherman's suggestion eliminated the need for the new
> parameter. Here's the updated webrev:
> http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev03/
>
Lance Andersen|
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 6/26/18, 6:54 AM, Alan Bateman wrote: On 26/06/2018 05:50, Joe Wang wrote: Hi Alan, Sherman, Here's a version where we, as Sherman suggested, throw an IAE with CCE as the cause. This approach reduces code duplication in SC, although it complicates the impl a little bit with the added parameter and the different behavior between the existing usages of the methods and the new ones. The existing code paths are kept intact so there's no compatibility issue for the existing code. This version also did not remove the try-catch in Files as Alan suggested earlier. http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev02/ This version looks much better. In StringCoding, do you really need throwCCE? The encode/decode methods do a replace or throw so I assume one flag will do. If combined with Sherman suggestion then it would be minimal changes to StringCoding. It would be nice to get rid of the IAE completely but that is for another day. In Files then you don't need to check if cause is null before testing its type. Yes, combined with Sherman's suggestion eliminated the need for the new parameter. Here's the updated webrev: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev03/ The update tests to check for UnmappedCharacterException and MalformedInputException look good. Thanks, Joe -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Sherman,
Combining the msg and cause is a great idea! That allowed us to satisfy
the existing code without changes/additional parameter and also the new
method for a CCE. I also moved the iae-catch into StringCoding, that
makes Files clean of such things.
Here's an updated webrev with both suggestions:
http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev03/
Thanks,
Joe
On 6/25/18, 10:28 PM, Xueming Shen wrote:
Hi Joe,
I would suggest always embed a malformed exception into the iae as showed
below, then the extra flag is no longer needed.
private static void throwMalformed(int off, int nb) {
String msg = "malformed input off : " + off + ", length : "
+ nb;
throw new IllegalArgumentException(msg, new
MalformedInputException(nb));
}
It's an implementation details, so we might be able to get rid of the
iae later if we
can figure out the better way to pass the malformed exception for both
ZipCoder/Files
later, without touch the api.
An alternative is to move the iae-catch into StringCoding, with pair
of similar methods
to throw IOE (to add into the JLA), not sure if it's better though. It
makes the Files
impl cleaner at least.
-Sherman
On 6/25/18, 9:50 PM, Joe Wang wrote:
Hi Alan, Sherman,
Here's a version where we, as Sherman suggested, throw an IAE with
CCE as the cause. This approach reduces code duplication in SC,
although it complicates the impl a little bit with the added
parameter and the different behavior between the existing usages of
the methods and the new ones. The existing code paths are kept intact
so there's no compatibility issue for the existing code.
This version also did not remove the try-catch in Files as Alan
suggested earlier.
http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev02/
Thanks,
Joe
On 6/25/18, 3:41 PM, Joe Wang wrote:
Hi Alan,
The test testMalformedRead and testMalformedWrite now expect an
UnmappableCharacterException instead of IOE:
webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev01/
Thanks,
Joe
On 6/25/18, 9:48 AM, Joe Wang wrote:
On 6/24/18, 12:11 PM, Alan Bateman wrote:
On 20/06/2018 04:32, Joe Wang wrote:
Thanks Alan. I created 8205058 to capture your suggestions.
Please see below for more details.
Changed the internal APIs to throw CCE instead. In the same way
as the previous changeset for 8201276, these methods are made
specific for the use cases (though they are now for
Files.read/writeString only) so that they are not mixed up with
existing ones that may inadvertently affect other usages.
One thing to note is that MalformedInputException or
UnmappableCharacterException will lose one piece of information
in comparison to the existing IAE, that is where it happens
(offset). Should there be an improvement in the future, we could
consider add it back to this part of code.
JBS: https://bugs.openjdk.java.net/browse/JDK-8205058
webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/
I see your point on MalformedInputException and
UnmappableCharacterException so maybe we can submit a new issue to
track the follow-on work.
Submitted: https://bugs.openjdk.java.net/browse/JDK-8205613
The update means a lot of duplication in StringCoding. Did you
consider (or measure) having the private encode/decode methods
take a parameter to indicate the exception handling? Sherman might
have suggestions on this.
I did. I didn't like the code duplication at all. But it would be
even messier if we reuse the code since the previous usage didn't
declare checked exception, but did capture the position (offset)
and length information, which means the new method while
unnecessary to capture these information for Files read/writeString
would still need to save them in case Exception occurs. Because of
the complication, I thought you and Sherman would again prefer a
specific methods than adding parameters (need fields as well).
Furthermore, in the first round of the review, Sherman mentioned
that he's been working on an improvement in this area. But I'll
wait till Sherman could comment on this.
In the tests, shouldn't testMalformedRead and testMalformedWrite
be updated so that expectedExceptions lists the specify exception
that is expected? If I read the update correctly then isn't
checking for MalformedInputException and
UnmappableCharacterException anywhere (it passes if IOException is
thrown).
MalformedInputException and UnmappableCharacterException are
implementation details, the tests only verified what the spec
required (IOE).
-Joe
-Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 26/06/2018 05:50, Joe Wang wrote: Hi Alan, Sherman, Here's a version where we, as Sherman suggested, throw an IAE with CCE as the cause. This approach reduces code duplication in SC, although it complicates the impl a little bit with the added parameter and the different behavior between the existing usages of the methods and the new ones. The existing code paths are kept intact so there's no compatibility issue for the existing code. This version also did not remove the try-catch in Files as Alan suggested earlier. http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev02/ This version looks much better. In StringCoding, do you really need throwCCE? The encode/decode methods do a replace or throw so I assume one flag will do. If combined with Sherman suggestion then it would be minimal changes to StringCoding. It would be nice to get rid of the IAE completely but that is for another day. In Files then you don't need to check if cause is null before testing its type. The update tests to check for UnmappedCharacterException and MalformedInputException look good. -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Joe,
I would suggest always embed a malformed exception into the iae as showed
below, then the extra flag is no longer needed.
private static void throwMalformed(int off, int nb) {
String msg = "malformed input off : " + off + ", length : " + nb;
throw new IllegalArgumentException(msg, new
MalformedInputException(nb));
}
It's an implementation details, so we might be able to get rid of the
iae later if we
can figure out the better way to pass the malformed exception for both
ZipCoder/Files
later, without touch the api.
An alternative is to move the iae-catch into StringCoding, with pair of
similar methods
to throw IOE (to add into the JLA), not sure if it's better though. It
makes the Files
impl cleaner at least.
-Sherman
On 6/25/18, 9:50 PM, Joe Wang wrote:
Hi Alan, Sherman,
Here's a version where we, as Sherman suggested, throw an IAE with CCE
as the cause. This approach reduces code duplication in SC, although
it complicates the impl a little bit with the added parameter and the
different behavior between the existing usages of the methods and the
new ones. The existing code paths are kept intact so there's no
compatibility issue for the existing code.
This version also did not remove the try-catch in Files as Alan
suggested earlier.
http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev02/
Thanks,
Joe
On 6/25/18, 3:41 PM, Joe Wang wrote:
Hi Alan,
The test testMalformedRead and testMalformedWrite now expect an
UnmappableCharacterException instead of IOE:
webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev01/
Thanks,
Joe
On 6/25/18, 9:48 AM, Joe Wang wrote:
On 6/24/18, 12:11 PM, Alan Bateman wrote:
On 20/06/2018 04:32, Joe Wang wrote:
Thanks Alan. I created 8205058 to capture your suggestions.
Please see below for more details.
Changed the internal APIs to throw CCE instead. In the same way as
the previous changeset for 8201276, these methods are made
specific for the use cases (though they are now for
Files.read/writeString only) so that they are not mixed up with
existing ones that may inadvertently affect other usages.
One thing to note is that MalformedInputException or
UnmappableCharacterException will lose one piece of information in
comparison to the existing IAE, that is where it happens (offset).
Should there be an improvement in the future, we could consider
add it back to this part of code.
JBS: https://bugs.openjdk.java.net/browse/JDK-8205058
webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/
I see your point on MalformedInputException and
UnmappableCharacterException so maybe we can submit a new issue to
track the follow-on work.
Submitted: https://bugs.openjdk.java.net/browse/JDK-8205613
The update means a lot of duplication in StringCoding. Did you
consider (or measure) having the private encode/decode methods take
a parameter to indicate the exception handling? Sherman might have
suggestions on this.
I did. I didn't like the code duplication at all. But it would be
even messier if we reuse the code since the previous usage didn't
declare checked exception, but did capture the position (offset) and
length information, which means the new method while unnecessary to
capture these information for Files read/writeString would still
need to save them in case Exception occurs. Because of the
complication, I thought you and Sherman would again prefer a
specific methods than adding parameters (need fields as well).
Furthermore, in the first round of the review, Sherman mentioned
that he's been working on an improvement in this area. But I'll wait
till Sherman could comment on this.
In the tests, shouldn't testMalformedRead and testMalformedWrite be
updated so that expectedExceptions lists the specify exception that
is expected? If I read the update correctly then isn't checking for
MalformedInputException and UnmappableCharacterException anywhere
(it passes if IOException is thrown).
MalformedInputException and UnmappableCharacterException are
implementation details, the tests only verified what the spec
required (IOE).
-Joe
-Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Alan, Sherman, Here's a version where we, as Sherman suggested, throw an IAE with CCE as the cause. This approach reduces code duplication in SC, although it complicates the impl a little bit with the added parameter and the different behavior between the existing usages of the methods and the new ones. The existing code paths are kept intact so there's no compatibility issue for the existing code. This version also did not remove the try-catch in Files as Alan suggested earlier. http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev02/ Thanks, Joe On 6/25/18, 3:41 PM, Joe Wang wrote: Hi Alan, The test testMalformedRead and testMalformedWrite now expect an UnmappableCharacterException instead of IOE: webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev01/ Thanks, Joe On 6/25/18, 9:48 AM, Joe Wang wrote: On 6/24/18, 12:11 PM, Alan Bateman wrote: On 20/06/2018 04:32, Joe Wang wrote: Thanks Alan. I created 8205058 to capture your suggestions. Please see below for more details. Changed the internal APIs to throw CCE instead. In the same way as the previous changeset for 8201276, these methods are made specific for the use cases (though they are now for Files.read/writeString only) so that they are not mixed up with existing ones that may inadvertently affect other usages. One thing to note is that MalformedInputException or UnmappableCharacterException will lose one piece of information in comparison to the existing IAE, that is where it happens (offset). Should there be an improvement in the future, we could consider add it back to this part of code. JBS: https://bugs.openjdk.java.net/browse/JDK-8205058 webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/ I see your point on MalformedInputException and UnmappableCharacterException so maybe we can submit a new issue to track the follow-on work. Submitted: https://bugs.openjdk.java.net/browse/JDK-8205613 The update means a lot of duplication in StringCoding. Did you consider (or measure) having the private encode/decode methods take a parameter to indicate the exception handling? Sherman might have suggestions on this. I did. I didn't like the code duplication at all. But it would be even messier if we reuse the code since the previous usage didn't declare checked exception, but did capture the position (offset) and length information, which means the new method while unnecessary to capture these information for Files read/writeString would still need to save them in case Exception occurs. Because of the complication, I thought you and Sherman would again prefer a specific methods than adding parameters (need fields as well). Furthermore, in the first round of the review, Sherman mentioned that he's been working on an improvement in this area. But I'll wait till Sherman could comment on this. In the tests, shouldn't testMalformedRead and testMalformedWrite be updated so that expectedExceptions lists the specify exception that is expected? If I read the update correctly then isn't checking for MalformedInputException and UnmappableCharacterException anywhere (it passes if IOException is thrown). MalformedInputException and UnmappableCharacterException are implementation details, the tests only verified what the spec required (IOE). -Joe -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Alan, The test testMalformedRead and testMalformedWrite now expect an UnmappableCharacterException instead of IOE: webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev01/ Thanks, Joe On 6/25/18, 9:48 AM, Joe Wang wrote: On 6/24/18, 12:11 PM, Alan Bateman wrote: On 20/06/2018 04:32, Joe Wang wrote: Thanks Alan. I created 8205058 to capture your suggestions. Please see below for more details. Changed the internal APIs to throw CCE instead. In the same way as the previous changeset for 8201276, these methods are made specific for the use cases (though they are now for Files.read/writeString only) so that they are not mixed up with existing ones that may inadvertently affect other usages. One thing to note is that MalformedInputException or UnmappableCharacterException will lose one piece of information in comparison to the existing IAE, that is where it happens (offset). Should there be an improvement in the future, we could consider add it back to this part of code. JBS: https://bugs.openjdk.java.net/browse/JDK-8205058 webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/ I see your point on MalformedInputException and UnmappableCharacterException so maybe we can submit a new issue to track the follow-on work. Submitted: https://bugs.openjdk.java.net/browse/JDK-8205613 The update means a lot of duplication in StringCoding. Did you consider (or measure) having the private encode/decode methods take a parameter to indicate the exception handling? Sherman might have suggestions on this. I did. I didn't like the code duplication at all. But it would be even messier if we reuse the code since the previous usage didn't declare checked exception, but did capture the position (offset) and length information, which means the new method while unnecessary to capture these information for Files read/writeString would still need to save them in case Exception occurs. Because of the complication, I thought you and Sherman would again prefer a specific methods than adding parameters (need fields as well). Furthermore, in the first round of the review, Sherman mentioned that he's been working on an improvement in this area. But I'll wait till Sherman could comment on this. In the tests, shouldn't testMalformedRead and testMalformedWrite be updated so that expectedExceptions lists the specify exception that is expected? If I read the update correctly then isn't checking for MalformedInputException and UnmappableCharacterException anywhere (it passes if IOException is thrown). MalformedInputException and UnmappableCharacterException are implementation details, the tests only verified what the spec required (IOE). -Joe -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 6/24/18, 12:11 PM, Alan Bateman wrote: On 20/06/2018 04:32, Joe Wang wrote: Thanks Alan. I created 8205058 to capture your suggestions. Please see below for more details. Changed the internal APIs to throw CCE instead. In the same way as the previous changeset for 8201276, these methods are made specific for the use cases (though they are now for Files.read/writeString only) so that they are not mixed up with existing ones that may inadvertently affect other usages. One thing to note is that MalformedInputException or UnmappableCharacterException will lose one piece of information in comparison to the existing IAE, that is where it happens (offset). Should there be an improvement in the future, we could consider add it back to this part of code. JBS: https://bugs.openjdk.java.net/browse/JDK-8205058 webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/ I see your point on MalformedInputException and UnmappableCharacterException so maybe we can submit a new issue to track the follow-on work. Submitted: https://bugs.openjdk.java.net/browse/JDK-8205613 The update means a lot of duplication in StringCoding. Did you consider (or measure) having the private encode/decode methods take a parameter to indicate the exception handling? Sherman might have suggestions on this. I did. I didn't like the code duplication at all. But it would be even messier if we reuse the code since the previous usage didn't declare checked exception, but did capture the position (offset) and length information, which means the new method while unnecessary to capture these information for Files read/writeString would still need to save them in case Exception occurs. Because of the complication, I thought you and Sherman would again prefer a specific methods than adding parameters (need fields as well). Furthermore, in the first round of the review, Sherman mentioned that he's been working on an improvement in this area. But I'll wait till Sherman could comment on this. In the tests, shouldn't testMalformedRead and testMalformedWrite be updated so that expectedExceptions lists the specify exception that is expected? If I read the update correctly then isn't checking for MalformedInputException and UnmappableCharacterException anywhere (it passes if IOException is thrown). MalformedInputException and UnmappableCharacterException are implementation details, the tests only verified what the spec required (IOE). -Joe -Alan
Re: RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 20/06/2018 04:32, Joe Wang wrote: Thanks Alan. I created 8205058 to capture your suggestions. Please see below for more details. Changed the internal APIs to throw CCE instead. In the same way as the previous changeset for 8201276, these methods are made specific for the use cases (though they are now for Files.read/writeString only) so that they are not mixed up with existing ones that may inadvertently affect other usages. One thing to note is that MalformedInputException or UnmappableCharacterException will lose one piece of information in comparison to the existing IAE, that is where it happens (offset). Should there be an improvement in the future, we could consider add it back to this part of code. JBS: https://bugs.openjdk.java.net/browse/JDK-8205058 webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/ I see your point on MalformedInputException and UnmappableCharacterException so maybe we can submit a new issue to track the follow-on work. The update means a lot of duplication in StringCoding. Did you consider (or measure) having the private encode/decode methods take a parameter to indicate the exception handling? Sherman might have suggestions on this. In the tests, shouldn't testMalformedRead and testMalformedWrite be updated so that expectedExceptions lists the specify exception that is expected? If I read the update correctly then isn't checking for MalformedInputException and UnmappableCharacterException anywhere (it passes if IOException is thrown). -Alan
RFR(JDK11/NIO) 8205058 throw CharacterCodingException --> Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Thanks Alan. I created 8205058 to capture your suggestions. Please see below for more details. On 6/14/18, 4:30 AM, Alan Bateman wrote: On 12/06/2018 17:52, Joe Wang wrote: Hi all, It's been a while since the 1st round of reviews. Thank you all for the valuable comments and suggestions! Note that Roger's last response went to nio-dev, but not core-libs-dev, I've therefore attached it below. CSR [2]: the CSR is now approved. Note the write method has been changed to writeString. Impl [3]: for performance reason and the different requirement from byte-string conversion for malformed/unmappable character handing, the implementation uses a specific method separate from the existing ones. Both Sherman and Alan preferred specific method than adding parameters to the APIs that might be error prone. Thanks Sherman for the code! JMH benchmark tests showed the new read method performs better than an operation that reads the bytes and convert to string. The gains start to be significant even at quite reasonable sizes (< 10K). [1] JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8202055 [3] webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ Please review. The javadoc looks good. One part of the implementation that could be cleaner is the exception thrown for the malformed or unmappable cases. There are sub-classes of CharacterCodingException defined for this that would be clearer than an IOException with an IllegalArgumentException as cause. Changed the internal APIs to throw CCE instead. In the same way as the previous changeset for 8201276, these methods are made specific for the use cases (though they are now for Files.read/writeString only) so that they are not mixed up with existing ones that may inadvertently affect other usages. One thing to note is that MalformedInputException or UnmappableCharacterException will lose one piece of information in comparison to the existing IAE, that is where it happens (offset). Should there be an improvement in the future, we could consider add it back to this part of code. A minor nit in Files is that you can import jdk.internal.misc.JavaLangAccess rather than repeating the fully qualified class name. You can also move the definition of JLA up to the top. There's also a few inconsistencies with the existing code that would be goof to fix too (indenting and line length issues mostly). Moved JLA and others to the top. Fixed also the indentations (mostly method declarations) and a few long lines. The test looks reasonable. In getData() then then "shouldn't happen" case should throw an exception as a NPE here might be tricky to diagnose there. Another nit is the sb field - can that be removed. Fixed too. JBS: https://bugs.openjdk.java.net/browse/JDK-8205058 webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8205058/webrev/ Regards, Joe -Alan
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 12/06/2018 17:52, Joe Wang wrote: Hi all, It's been a while since the 1st round of reviews. Thank you all for the valuable comments and suggestions! Note that Roger's last response went to nio-dev, but not core-libs-dev, I've therefore attached it below. CSR [2]: the CSR is now approved. Note the write method has been changed to writeString. Impl [3]: for performance reason and the different requirement from byte-string conversion for malformed/unmappable character handing, the implementation uses a specific method separate from the existing ones. Both Sherman and Alan preferred specific method than adding parameters to the APIs that might be error prone. Thanks Sherman for the code! JMH benchmark tests showed the new read method performs better than an operation that reads the bytes and convert to string. The gains start to be significant even at quite reasonable sizes (< 10K). [1] JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8202055 [3] webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ Please review. The javadoc looks good. One part of the implementation that could be cleaner is the exception thrown for the malformed or unmappable cases. There are sub-classes of CharacterCodingException defined for this that would be clearer than an IOException with an IllegalArgumentException as cause. A minor nit in Files is that you can import jdk.internal.misc.JavaLangAccess rather than repeating the fully qualified class name. You can also move the definition of JLA up to the top. There's also a few inconsistencies with the existing code that would be goof to fix too (indenting and line length issues mostly). The test looks reasonable. In getData() then then "shouldn't happen" case should throw an exception as a NPE here might be tricky to diagnose there. Another nit is the sb field - can that be removed. -Alan
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Thanks Roger! I pushed the changeset after s/unmappble/unmappable, it found three of them :-) Best, Joe On 6/12/18, 10:37 AM, Roger Riggs wrote: Hi Joe, Looks good to me. Typo: StringCoding.java:1026 "unmappble" (no new webrev needed) Regards, Roger On 6/12/2018 12:52 PM, Joe Wang wrote: Hi all, It's been a while since the 1st round of reviews. Thank you all for the valuable comments and suggestions! Note that Roger's last response went to nio-dev, but not core-libs-dev, I've therefore attached it below. CSR [2]: the CSR is now approved. Note the write method has been changed to writeString. Impl [3]: for performance reason and the different requirement from byte-string conversion for malformed/unmappable character handing, the implementation uses a specific method separate from the existing ones. Both Sherman and Alan preferred specific method than adding parameters to the APIs that might be error prone. Thanks Sherman for the code! JMH benchmark tests showed the new read method performs better than an operation that reads the bytes and convert to string. The gains start to be significant even at quite reasonable sizes (< 10K). [1] JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8202055 [3] webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ Please review. Thanks, Joe On 5/15/18, 7:51 AM, Roger Riggs wrote: Hi Joe, et. al. My $.02 on line separators: - We should avoid clever tricks trying to solve problems that are infrequent such as cross OS file line operations. Most files will be read/written on a single system so the line separators will be as expected. - Have/use APIs that split lines consistently accepting both line separators so developer code can be agnostic to line separators. aka BufferedReader.readLine for developers that are processing the contents *as lines*. Those other methods already exist; if there are any gaps in line oriented processing that's a separate task. - These new file methods are defined to handle Charset encoding/decoding and buffering. Since there are other methods to deal with files as lines these methods should not look for or break to lines. - Performance: adding code to look for line characters will slow it down and in most cases would have no impact since the line endings will match the system. - The strings passed to writeString (CharSequence) should have been constructed using the proper line separators. There are any number of methods that insert the os specific line terminator and its easy to write File.separator as needed. As for the method naming: I'd prefer "writeString" as a familiar term that isn't stretched too far by making the argument be a CharSequence. its fine to call a CharSequence a string (with a lower case s). $.02, Roger On 4/27/18 6:33 PM, Joe Wang wrote: On 4/27/2018 12:50 PM, fo...@univ-mlv.fr wrote: - Mail original - De: "Joe Wang" À: "Remi Forax" , "Alan Bateman" Cc: "nio-dev" , "core-libs-dev" Envoyé: Vendredi 27 Avril 2018 21:21:01 Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file Hi Rémi, Alan, Hi Joe, I'm not sure we'd want to replace system dependent line separators with '\n'. There are cases as you described where the replacement makes sense. However, there are other cases too. For example, the purpose is to read, edit a text file and then write it back. If this is on Windows, and if line separators are replaced with '\n', that would cause the resulting file not formatted properly in for example Notepad. There are cases where users write text files on Windows using Java, and only found the lines were not separated in Notepad. I agree that why the counterpart of readString() that write the string should inserts the platform's line separator. BTW, i just found that those methods are not named writeString, or writeCharSequence, i think they should. While readString() does not modify the original content (e.g. by replacing the platform's line separator with '\n'), write(String) won't either, by adding extra characters such as the line separator. I would think interfaces shall read along with the parameters. readString(Path) == read as a String from the specified Path (one could argue for readToString, readAsString, but we generally omit the preps) write(Path, CharSequence) == write the CharSequence to the file, since CharSequence is already in the method signature as a parameter, we probably don't want to add that to the name, otherwise it would read like repeating the word CharSequence. It is in a similar situation as write(Path, Iterable) where it was defined as writeLines(Path, Iterable). Files.write(Pa
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Joe, Looks good to me. Typo: StringCoding.java:1026 "unmappble" (no new webrev needed) Regards, Roger On 6/12/2018 12:52 PM, Joe Wang wrote: Hi all, It's been a while since the 1st round of reviews. Thank you all for the valuable comments and suggestions! Note that Roger's last response went to nio-dev, but not core-libs-dev, I've therefore attached it below. CSR [2]: the CSR is now approved. Note the write method has been changed to writeString. Impl [3]: for performance reason and the different requirement from byte-string conversion for malformed/unmappable character handing, the implementation uses a specific method separate from the existing ones. Both Sherman and Alan preferred specific method than adding parameters to the APIs that might be error prone. Thanks Sherman for the code! JMH benchmark tests showed the new read method performs better than an operation that reads the bytes and convert to string. The gains start to be significant even at quite reasonable sizes (< 10K). [1] JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8202055 [3] webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ Please review. Thanks, Joe On 5/15/18, 7:51 AM, Roger Riggs wrote: Hi Joe, et. al. My $.02 on line separators: - We should avoid clever tricks trying to solve problems that are infrequent such as cross OS file line operations. Most files will be read/written on a single system so the line separators will be as expected. - Have/use APIs that split lines consistently accepting both line separators so developer code can be agnostic to line separators. aka BufferedReader.readLine for developers that are processing the contents *as lines*. Those other methods already exist; if there are any gaps in line oriented processing that's a separate task. - These new file methods are defined to handle Charset encoding/decoding and buffering. Since there are other methods to deal with files as lines these methods should not look for or break to lines. - Performance: adding code to look for line characters will slow it down and in most cases would have no impact since the line endings will match the system. - The strings passed to writeString (CharSequence) should have been constructed using the proper line separators. There are any number of methods that insert the os specific line terminator and its easy to write File.separator as needed. As for the method naming: I'd prefer "writeString" as a familiar term that isn't stretched too far by making the argument be a CharSequence. its fine to call a CharSequence a string (with a lower case s). $.02, Roger On 4/27/18 6:33 PM, Joe Wang wrote: On 4/27/2018 12:50 PM, fo...@univ-mlv.fr wrote: - Mail original - De: "Joe Wang" À: "Remi Forax" , "Alan Bateman" Cc: "nio-dev" , "core-libs-dev" Envoyé: Vendredi 27 Avril 2018 21:21:01 Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file Hi Rémi, Alan, Hi Joe, I'm not sure we'd want to replace system dependent line separators with '\n'. There are cases as you described where the replacement makes sense. However, there are other cases too. For example, the purpose is to read, edit a text file and then write it back. If this is on Windows, and if line separators are replaced with '\n', that would cause the resulting file not formatted properly in for example Notepad. There are cases where users write text files on Windows using Java, and only found the lines were not separated in Notepad. I agree that why the counterpart of readString() that write the string should inserts the platform's line separator. BTW, i just found that those methods are not named writeString, or writeCharSequence, i think they should. While readString() does not modify the original content (e.g. by replacing the platform's line separator with '\n'), write(String) won't either, by adding extra characters such as the line separator. I would think interfaces shall read along with the parameters. readString(Path) == read as a String from the specified Path (one could argue for readToString, readAsString, but we generally omit the preps) write(Path, CharSequence) == write the CharSequence to the file, since CharSequence is already in the method signature as a parameter, we probably don't want to add that to the name, otherwise it would read like repeating the word CharSequence. It is in a similar situation as write(Path, Iterable) where it was defined as writeLines(Path, Iterable). Files.write(Path, Iterable) is also specified to terminate each line with the platform's line separator. If readString does the replacement, it would be incons
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi all, It's been a while since the 1st round of reviews. Thank you all for the valuable comments and suggestions! Note that Roger's last response went to nio-dev, but not core-libs-dev, I've therefore attached it below. CSR [2]: the CSR is now approved. Note the write method has been changed to writeString. Impl [3]: for performance reason and the different requirement from byte-string conversion for malformed/unmappable character handing, the implementation uses a specific method separate from the existing ones. Both Sherman and Alan preferred specific method than adding parameters to the APIs that might be error prone. Thanks Sherman for the code! JMH benchmark tests showed the new read method performs better than an operation that reads the bytes and convert to string. The gains start to be significant even at quite reasonable sizes (< 10K). [1] JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 [2] CSR: https://bugs.openjdk.java.net/browse/JDK-8202055 [3] webrevs: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ Please review. Thanks, Joe On 5/15/18, 7:51 AM, Roger Riggs wrote: Hi Joe, et. al. My $.02 on line separators: - We should avoid clever tricks trying to solve problems that are infrequent such as cross OS file line operations. Most files will be read/written on a single system so the line separators will be as expected. - Have/use APIs that split lines consistently accepting both line separators so developer code can be agnostic to line separators. aka BufferedReader.readLine for developers that are processing the contents *as lines*. Those other methods already exist; if there are any gaps in line oriented processing that's a separate task. - These new file methods are defined to handle Charset encoding/decoding and buffering. Since there are other methods to deal with files as lines these methods should not look for or break to lines. - Performance: adding code to look for line characters will slow it down and in most cases would have no impact since the line endings will match the system. - The strings passed to writeString (CharSequence) should have been constructed using the proper line separators. There are any number of methods that insert the os specific line terminator and its easy to write File.separator as needed. As for the method naming: I'd prefer "writeString" as a familiar term that isn't stretched too far by making the argument be a CharSequence. its fine to call a CharSequence a string (with a lower case s). $.02, Roger On 4/27/18 6:33 PM, Joe Wang wrote: On 4/27/2018 12:50 PM, fo...@univ-mlv.fr wrote: - Mail original - De: "Joe Wang" À: "Remi Forax" , "Alan Bateman" Cc: "nio-dev" , "core-libs-dev" Envoyé: Vendredi 27 Avril 2018 21:21:01 Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file Hi Rémi, Alan, Hi Joe, I'm not sure we'd want to replace system dependent line separators with '\n'. There are cases as you described where the replacement makes sense. However, there are other cases too. For example, the purpose is to read, edit a text file and then write it back. If this is on Windows, and if line separators are replaced with '\n', that would cause the resulting file not formatted properly in for example Notepad. There are cases where users write text files on Windows using Java, and only found the lines were not separated in Notepad. I agree that why the counterpart of readString() that write the string should inserts the platform's line separator. BTW, i just found that those methods are not named writeString, or writeCharSequence, i think they should. While readString() does not modify the original content (e.g. by replacing the platform's line separator with '\n'), write(String) won't either, by adding extra characters such as the line separator. I would think interfaces shall read along with the parameters. readString(Path) == read as a String from the specified Path (one could argue for readToString, readAsString, but we generally omit the preps) write(Path, CharSequence) == write the CharSequence to the file, since CharSequence is already in the method signature as a parameter, we probably don't want to add that to the name, otherwise it would read like repeating the word CharSequence. It is in a similar situation as write(Path, Iterable) where it was defined as writeLines(Path, Iterable). Files.write(Path, Iterable) is also specified to terminate each line with the platform's line separator. If readString does the replacement, it would be inconsistent. Anyway, if we look for consistency the methods writeCharSequence should transform the \n in the CharSequence to the platform's line separator.
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Hamlin, Thanks for reviewing the test! Fixed now, by calling deleteOnExit. Indeed, I didn't realize I got a few empty tmp files :-) Best, Joe On 4/28/2018 12:35 AM, Hamlin Li wrote: Hi Joe, just a minor comment about the test in ReadWriteString.java. For both testMalformedWrite and testMalformedRead, temp files are not deleted, as Files.deleteIfExists(path); is skipped by the expected IOException. File.deleteOnExit() may help. Thank you -Hamlin On 27/04/2018 12:50 PM, Joe Wang wrote: Hi, We're looking into adding methods to Files to read a file into a String/write a String to a file. Below is the current proposal. Please review. JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ specdiff: http://cr.openjdk.java.net/~joehw/jdk11/8201276/specdiff/java/nio/file/Files.html Thanks, Joe
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Joe, just a minor comment about the test in ReadWriteString.java. For both testMalformedWrite and testMalformedRead, temp files are not deleted, as Files.deleteIfExists(path); is skipped by the expected IOException. File.deleteOnExit() may help. Thank you -Hamlin On 27/04/2018 12:50 PM, Joe Wang wrote: Hi, We're looking into adding methods to Files to read a file into a String/write a String to a file. Below is the current proposal. Please review. JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ specdiff: http://cr.openjdk.java.net/~joehw/jdk11/8201276/specdiff/java/nio/file/Files.html Thanks, Joe
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 4/27/2018 12:50 PM, fo...@univ-mlv.fr wrote:
- Mail original -
De: "Joe Wang"
À: "Remi Forax" , "Alan Bateman"
Cc: "nio-dev" , "core-libs-dev"
Envoyé: Vendredi 27 Avril 2018 21:21:01
Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for
reading/writing a string from/to a file
Hi Rémi, Alan,
Hi Joe,
I'm not sure we'd want to replace system dependent line separators with
'\n'. There are cases as you described where the replacement makes
sense. However, there are other cases too. For example, the purpose is
to read, edit a text file and then write it back. If this is on Windows,
and if line separators are replaced with '\n', that would cause the
resulting file not formatted properly in for example Notepad. There are
cases where users write text files on Windows using Java, and only found
the lines were not separated in Notepad.
I agree that why the counterpart of readString() that write the string should
inserts the platform's line separator.
BTW, i just found that those methods are not named writeString, or
writeCharSequence, i think they should.
While readString() does not modify the original content (e.g. by
replacing the platform's line separator with '\n'), write(String) won't
either, by adding extra characters such as the line separator.
I would think interfaces shall read along with the parameters.
readString(Path) == read as a String from the specified Path (one
could argue for readToString, readAsString, but we generally omit the preps)
write(Path, CharSequence) == write the CharSequence to the file,
since CharSequence is already in the method signature as a parameter, we
probably don't want to add that to the name, otherwise it would read
like repeating the word CharSequence.
It is in a similar situation as write(Path, Iterable) where it was
defined as writeLines(Path, Iterable).
Files.write(Path, Iterable) is also specified to terminate each line
with the platform's line separator. If readString does the replacement,
it would be inconsistent.
Anyway, if we look for consistency the methods writeCharSequence should
transform the \n in the CharSequence to the platform's line separator.
Files.write(Path, Iterable) is is not a counterpart of readString(), it's
consistent with Files.lines() or Files.readLines() (or
BufferedReader.readLine()) that all suppress the line separators. Anyway,
Files.write(path, readString(path)::line) will be consistent if you replace the
line separators or not because String.line() suppresses the line separators.
readString pairs with write(String), therefore it's more like
Files.write(path, readString(path)) than readString(path)::line. The use
case:
String s = Files.read(path);
Files.write(path, s.replace("/config/location", "/new/location"));
would then work as expected.
These two methods are one-off (open/read/write/close file) operation.
write(String) therefore is not intended for adding/appending multiple
Strings from other operations such as String.line(). If an app needs to
put the result of String.line() or any other processes into a file using
this method, it needs to take care of adding the line separator itself
when necessary. "when necessary" would be a judgement call by the developer.
That said, if there's a consensus on the idea of terminating the string
with a line separator, then readString method would need to strip it off
to go with the write method.
I would think readString behaves like readAllBytes, that would simply read all
content faithfully.
readString does an interpolation due to the Charset so it's not the real
content, again, the idea is that developers should not have to care about the
platform's line separators (they have more important things to think).
The other solution is to just not introduce those new methods, after all
Files.lines().collect(Collectors.joining("\n")) already does the job, no ?
While there are many ways to do it, none is as straight-forward. "Read
(entire) file to String"/"Write String to file" are popular requests
from users. Read to string -> do some String manipulation -> write it
back is such a simple use case, it really needs to be very easy to do as
illustrated in the above code snippet.
Cheers,
Joe
Best,
Joe
regards,
Rémi
On 4/27/2018 4:43 AM, fo...@univ-mlv.fr wrote:
Hi Alan,
People do not read the documentation.
So adding something in the documentation about when a method should be used or
not is never a solution.
Here the user want a String and provides a charset so you have no way but to
decode the content to substitute the line separator.
cheers,
Rémi
----- Mail original -----
De: "Alan Bateman"
À: "Remi Forax" , "Joe Wang"
Cc: "nio-d
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
- Mail original -
> De: "Joe Wang"
> À: "Remi Forax" , "Alan Bateman"
> Cc: "nio-dev" , "core-libs-dev"
>
> Envoyé: Vendredi 27 Avril 2018 21:21:01
> Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for
> reading/writing a string from/to a file
> Hi Rémi, Alan,
Hi Joe,
>
> I'm not sure we'd want to replace system dependent line separators with
> '\n'. There are cases as you described where the replacement makes
> sense. However, there are other cases too. For example, the purpose is
> to read, edit a text file and then write it back. If this is on Windows,
> and if line separators are replaced with '\n', that would cause the
> resulting file not formatted properly in for example Notepad. There are
> cases where users write text files on Windows using Java, and only found
> the lines were not separated in Notepad.
I agree that why the counterpart of readString() that write the string should
inserts the platform's line separator.
BTW, i just found that those methods are not named writeString, or
writeCharSequence, i think they should.
>
> Files.write(Path, Iterable) is also specified to terminate each line
> with the platform's line separator. If readString does the replacement,
> it would be inconsistent.
Anyway, if we look for consistency the methods writeCharSequence should
transform the \n in the CharSequence to the platform's line separator.
Files.write(Path, Iterable) is is not a counterpart of readString(), it's
consistent with Files.lines() or Files.readLines() (or
BufferedReader.readLine()) that all suppress the line separators. Anyway,
Files.write(path, readString(path)::line) will be consistent if you replace the
line separators or not because String.line() suppresses the line separators.
>
> I would think readString behaves like readAllBytes, that would simply read
> all content faithfully.
readString does an interpolation due to the Charset so it's not the real
content, again, the idea is that developers should not have to care about the
platform's line separators (they have more important things to think).
The other solution is to just not introduce those new methods, after all
Files.lines().collect(Collectors.joining("\n")) already does the job, no ?
>
> Best,
> Joe
regards,
Rémi
>
> On 4/27/2018 4:43 AM, fo...@univ-mlv.fr wrote:
>> Hi Alan,
>>
>> People do not read the documentation.
>> So adding something in the documentation about when a method should be used
>> or
>> not is never a solution.
>>
>> Here the user want a String and provides a charset so you have no way but to
>> decode the content to substitute the line separator.
>>
>> cheers,
>> Rémi
>>
>> - Mail original -
>>> De: "Alan Bateman"
>>> À: "Remi Forax" , "Joe Wang"
>>> Cc: "nio-dev" , "core-libs-dev"
>>>
>>> Envoyé: Vendredi 27 Avril 2018 13:34:12
>>> Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for
>>> reading/writing a string from/to a file
>>> On 27/04/2018 12:29, Remi Forax wrote:
>>>> I think that having a readString that includes OS dependent line
>>>> separators is a
>>>> mistake,
>>>> Java does a great job to try to shield the developer from the kind of
>>>> things
>>>> that makes programs behave differently on different platforms.
>>>>
>>>> readString should subtitute (\r)?\n to \n otherwise either people will do
>>>> a call
>>>> replace() which is less efficient or will learn the lesson the hard way.
>>>>
>>>> raw string literal does the same substitution for the same reason.
>>>>
>>> Yes, there are several discussion points around this and somewhat timely
>>> with multi-string support.
>>>
>>> One thing that I think Joe will need in this API is some note to make it
>>> clearer what the intended usage is (as I think the intention is simple
>>> cases with mostly single lines of text).
>>>
> >> -Alan.
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Rémi, Alan, I'm not sure we'd want to replace system dependent line separators with '\n'. There are cases as you described where the replacement makes sense. However, there are other cases too. For example, the purpose is to read, edit a text file and then write it back. If this is on Windows, and if line separators are replaced with '\n', that would cause the resulting file not formatted properly in for example Notepad. There are cases where users write text files on Windows using Java, and only found the lines were not separated in Notepad. Files.write(Path, Iterable) is also specified to terminate each line with the platform's line separator. If readString does the replacement, it would be inconsistent. I would think readString behaves like readAllBytes, that would simply read all content faithfully. Best, Joe On 4/27/2018 4:43 AM, fo...@univ-mlv.fr wrote: Hi Alan, People do not read the documentation. So adding something in the documentation about when a method should be used or not is never a solution. Here the user want a String and provides a charset so you have no way but to decode the content to substitute the line separator. cheers, Rémi - Mail original - De: "Alan Bateman" À: "Remi Forax" , "Joe Wang" Cc: "nio-dev" , "core-libs-dev" Envoyé: Vendredi 27 Avril 2018 13:34:12 Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file On 27/04/2018 12:29, Remi Forax wrote: I think that having a readString that includes OS dependent line separators is a mistake, Java does a great job to try to shield the developer from the kind of things that makes programs behave differently on different platforms. readString should subtitute (\r)?\n to \n otherwise either people will do a call replace() which is less efficient or will learn the lesson the hard way. raw string literal does the same substitution for the same reason. Yes, there are several discussion points around this and somewhat timely with multi-string support. One thing that I think Joe will need in this API is some note to make it clearer what the intended usage is (as I think the intention is simple cases with mostly single lines of text). -Alan.
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 4/27/2018 4:13 AM, Alan Bateman wrote: On 27/04/2018 05:50, Joe Wang wrote: Hi, We're looking into adding methods to Files to read a file into a String/write a String to a file. Below is the current proposal. Please review. JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ specdiff: http://cr.openjdk.java.net/~joehw/jdk11/8201276/specdiff/java/nio/file/Files.html The javadoc for these 4 methods looks okay. It might be helpful to include something in the readString javadoc to make it absolutely clear that the String may include line separators. I assume the "Note that .." paragraph can be changed to an @apiNote. Added a statement to indicate that "the resulting string will contain line separators as they appear in the file". Changed the 'note' to @apiNote. I assume you'll add "@since 11" to the readString methods. Added. It would be good to keep the existing formatting/style consistent with the existing code if you can, e.g. tags, 4 space indent rather than 8 for the throws, etc. Fixed and the throws. I didn't even notice that the IDE (NetBeans) added 8 spaces! I can't tell from your mail if you are just looking for feedback on the current implementation + tests or just the API. I assume there are alternatives to using StringBuilder for the readString methods for example. Both. I thought webrevs would be helpful where specdiff was not clear enough, for example, you won't otherwise notice the formatting/style issue :-) I changed that to new String(readAllBytes(path), charset) as it's convenient with readAllBytes handling all situations (OOM and etc.). But alternative solution to avoid copying would be nice. -Joe -Alan
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi Alan, People do not read the documentation. So adding something in the documentation about when a method should be used or not is never a solution. Here the user want a String and provides a charset so you have no way but to decode the content to substitute the line separator. cheers, Rémi - Mail original - > De: "Alan Bateman" > À: "Remi Forax" , "Joe Wang" > Cc: "nio-dev" , "core-libs-dev" > > Envoyé: Vendredi 27 Avril 2018 13:34:12 > Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for > reading/writing a string from/to a file > On 27/04/2018 12:29, Remi Forax wrote: >> I think that having a readString that includes OS dependent line separators >> is a >> mistake, >> Java does a great job to try to shield the developer from the kind of things >> that makes programs behave differently on different platforms. >> >> readString should subtitute (\r)?\n to \n otherwise either people will do a >> call >> replace() which is less efficient or will learn the lesson the hard way. >> >> raw string literal does the same substitution for the same reason. >> > Yes, there are several discussion points around this and somewhat timely > with multi-string support. > > One thing that I think Joe will need in this API is some note to make it > clearer what the intended usage is (as I think the intention is simple > cases with mostly single lines of text). > > -Alan.
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 27/04/2018 12:29, Remi Forax wrote: I think that having a readString that includes OS dependent line separators is a mistake, Java does a great job to try to shield the developer from the kind of things that makes programs behave differently on different platforms. readString should subtitute (\r)?\n to \n otherwise either people will do a call replace() which is less efficient or will learn the lesson the hard way. raw string literal does the same substitution for the same reason. Yes, there are several discussion points around this and somewhat timely with multi-string support. One thing that I think Joe will need in this API is some note to make it clearer what the intended usage is (as I think the intention is simple cases with mostly single lines of text). -Alan.
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
I think that having a readString that includes OS dependent line separators is a mistake, Java does a great job to try to shield the developer from the kind of things that makes programs behave differently on different platforms. readString should subtitute (\r)?\n to \n otherwise either people will do a call replace() which is less efficient or will learn the lesson the hard way. raw string literal does the same substitution for the same reason. regards Rémi - Mail original - > De: "Alan Bateman" > À: "Joe Wang" > Cc: "nio-dev" , "core-libs-dev" > > Envoyé: Vendredi 27 Avril 2018 13:13:52 > Objet: Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for > reading/writing a string from/to a file > On 27/04/2018 05:50, Joe Wang wrote: >> Hi, >> >> We're looking into adding methods to Files to read a file into a >> String/write a String to a file. Below is the current proposal. Please >> review. >> >> JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 >> >> webrev: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ >> >> specdiff: >> http://cr.openjdk.java.net/~joehw/jdk11/8201276/specdiff/java/nio/file/Files.html > The javadoc for these 4 methods looks okay. It might be helpful to > include something in the readString javadoc to make it absolutely clear > that the String may include line separators. I assume the "Note that .." > paragraph can be changed to an @apiNote. > > I assume you'll add "@since 11" to the readString methods. > > It would be good to keep the existing formatting/style consistent with > the existing code if you can, e.g. tags, 4 space indent rather than > 8 for the throws, etc. > > I can't tell from your mail if you are just looking for feedback on the > current implementation + tests or just the API. I assume there are > alternatives to using StringBuilder for the readString methods for example. > > -Alan
Re: RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
On 27/04/2018 05:50, Joe Wang wrote: Hi, We're looking into adding methods to Files to read a file into a String/write a String to a file. Below is the current proposal. Please review. JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ specdiff: http://cr.openjdk.java.net/~joehw/jdk11/8201276/specdiff/java/nio/file/Files.html The javadoc for these 4 methods looks okay. It might be helpful to include something in the readString javadoc to make it absolutely clear that the String may include line separators. I assume the "Note that .." paragraph can be changed to an @apiNote. I assume you'll add "@since 11" to the readString methods. It would be good to keep the existing formatting/style consistent with the existing code if you can, e.g. tags, 4 space indent rather than 8 for the throws, etc. I can't tell from your mail if you are just looking for feedback on the current implementation + tests or just the API. I assume there are alternatives to using StringBuilder for the readString methods for example. -Alan
RFR (JDK11/NIO) 8201276: (fs) Add methods to Files for reading/writing a string from/to a file
Hi, We're looking into adding methods to Files to read a file into a String/write a String to a file. Below is the current proposal. Please review. JBS: https://bugs.openjdk.java.net/browse/JDK-8201276 webrev: http://cr.openjdk.java.net/~joehw/jdk11/8201276/webrev/ specdiff: http://cr.openjdk.java.net/~joehw/jdk11/8201276/specdiff/java/nio/file/Files.html Thanks, Joe
