On 07/10/14 21:50, Jason Mehrens wrote:

Hi Daniel,

The only thing I noticed is a missing @since tag on the FileHandler.

Oops, thanks for catching that Jason. -- daniel

Jason

----------------------------------------

...

Date: Tue, 7 Oct 2014 15:13:13 +0200 From: daniel.fuchs@oracle.com To: core-libs-dev@openjdk.java.net Subject: RFR: 8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written.

Hi,

Please find below a patch for:

8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written. https://bugs.openjdk.java.net/browse/JDK-8059767

webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.00/

This follows an issue reported on this list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-August/028280.html

The patch changes 'limit' from 'int' to 'long', fixes the handling of overflow, and adds a new constructor that allows to pass a long for 'limit'. It also makes it possible to specify a long value for 'limit' in the configuration.

The test checks the handling of overflow by tweaking the internal of MeteredStream through reflection. Not ideal, but I couldn't find any other way.

best regards,

-- daniel

On 08/10/14 08:26, Ivan Gerasimov wrote:

Hi Daniel!

Would it make sense to make the old constructor FileHandler(String, *int*, int, boolean) call the new one, casting the limit to long?

Hi Ivan, I was about to reply 'no' out of consistency with the other constructors - which don't call each others - when I realized that it was not be possible to make the other constructors call each others anyway - as passing a parameter to a constructor means overriding the default value read from the configuration file. This however doesn't hold in our case as both constructors have exactly the same code - so there's no reason for not making FileHandler(String, *int*, int, boolean) call FileHandler(String, *long*, int, boolean). Which now makes me question whether I should add a second constructor FileHandler(String, *long*, int) to shadow FileHandler(String, *int*, int) - as Stanimir aleready suggested. My feeling is that the case where you would want to rely on the default value of 'append' configured in the logging.properties file instead of specifying one to the constructor should be sufficiently rare to not warrant the addition of a second constructor. And if you really wanted to rely on that default value you could go and fetch it before calling the constructor. Anyway - good remark - and here is a new webrev including yours and Jason's comments. http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.01 best regards, -- daniel

Sincerely yours, Ivan

On 07.10.2014 17:13, Daniel Fuchs wrote:

...

Hi,

Please find below a patch for:

8059767: FileHandler should allow 'long' limits and handle overflow of MeteredStream.written. https://bugs.openjdk.java.net/browse/JDK-8059767

webrev: http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.00/

This follows an issue reported on this list: http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-August/028280.html

The patch changes 'limit' from 'int' to 'long', fixes the handling of overflow, and adds a new constructor that allows to pass a long for 'limit'. It also makes it possible to specify a long value for 'limit' in the configuration.

The test checks the handling of overflow by tweaking the internal of MeteredStream through reflection. Not ideal, but I couldn't find any other way.

best regards,

-- daniel

On 10/14/2014 3:55 AM, Daniel Fuchs wrote:

http://cr.openjdk.java.net/~dfuchs/webrev_8059767/webrev.02

Looks good. Nit: use {@code...} instead of <tt>...</tt>. It may be inconsistent with the javadoc for other existing constructors that I think it's good to clean up the new one at least. No need to generate a new webrev. Mandy