Looks good to me. Thanks for taking this on!
Jason ---------------------------------------- > Date: Fri, 27 Mar 2015 17:18:45 +0100 > From: daniel.fu...@oracle.com > To: jason_mehr...@hotmail.com; lance.ander...@oracle.com > CC: core-libs-dev@openjdk.java.net; roger.ri...@oracle.com > Subject: Re: RFR: 8075810: LogManager.readConfiguration may throw > undocumented IllegalArgumentException > > Hi, > > I received private feedback that using plain IOException > might be preferable to using CharConversionException, as > that doesn't exactly reflect what is going on here. > > So here is my new webrev - which includes the minor formatting > issues found by Jason, but reverts to using plain IOException: > > http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.02 > > If I don't hear negative feedback that's what I'm going to > present to CCC. > > best regards, > > -- daniel > > On 25/03/15 12:41, Daniel Fuchs wrote: >> Thanks for the review Jason! >> >> On 24/03/15 18:01, Jason Mehrens wrote: >>> Hi Daniel, >>> >>> Looks good. The only other alternative would be to use >>> java.io.CharConversionException over IOException. We could even >>> consider dropping the cause because the subclass of I/O exception >>> would convey the same meaning. >> >> Here is a an updated webrev where I use >> java.io.CharConversionException instead of plain IOException. >> >> http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.01/ >> >> Note: I had a look in the JDK sources and CharConversionException >> doesn't appear to be widely used. Is this the better alternative? >> I'd be happy with both (webrev.01 or webrev.00). >> Using CharConversionException means that we have to trust that >> Properties.load will obey its spec and only throw IAE in case >> of character conversion issues. >> >>> Minor formatting issues with a missing space after the catch keyword >> >> Done. Thanks for catching it. >> >>> and missing a tab ahead of 'props.load' >> >> That is an artifact of how webrev calls diff I think. >> If you look at the frames version you will see that the tab is here. >> >> best regards, >> >> -- daniel >> >>> >>> Jason >>> >>> >>> ---------------------------------------- >>>> Date: Tue, 24 Mar 2015 16:06:47 +0100 >>>> From: daniel.fu...@oracle.com >>>> To: lance.ander...@oracle.com >>>> Subject: Re: RFR: 8075810: LogManager.readConfiguration may throw >>>> undocumented IllegalArgumentException >>>> CC: core-libs-dev@openjdk.java.net >>>> >>>> Hi Lance, >>>> >>>> Thanks for the review! >>>> >>>> On 24/03/15 16:01, Lance Andersen wrote: >>>>> Hi Daniel, >>>>> >>>>> This looks OK but I might suggest clarifying that the cause could be >>>>> set >>>>> in the javadoc. Now that being said, I am not sure how consistent we >>>>> are across the JDK or just make the assumption people will check the >>>>> cause if they desire. >>>> >>>> Hmmm. I have the feeling that the best place for that would be i the >>>> release notes - rather than in the API doc (which reminds me I should >>>> plan to add some release note text to the issue). >>>> >>>>> As far as backporting, unless it really needed, I would probably would >>>>> not as a change such as this typically should be done as part of an >>>>> errata/MR (due to change of behavior and it is not that big of an >>>>> issue). >>>> >>>> Right, my thinking too. Thanks for sharing your opinion! >>>> >>>> best regards, >>>> >>>> -- daniel >>>> >>>>> >>>>> Best >>>>> Lance >>>>> On Mar 24, 2015, at 10:42 AM, Daniel Fuchs <daniel.fu...@oracle.com >>>>> <mailto:daniel.fu...@oracle.com>> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> Please find below a fix for >>>>>> >>>>>> 8075810: LogManager.readConfiguration may throw >>>>>> undocumented IllegalArgumentException >>>>>> >>>>>> https://bugs.openjdk.java.net/browse/JDK-8075810 >>>>>> >>>>>> The proposed fix is to catch the IllegalArgumentException and >>>>>> wrap it in an IOException, since LogManager.readConfiguration >>>>>> is specified to throw IOException "if there are problems reading >>>>>> from the stream." >>>>>> >>>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8075810/webrev.00/ >>>>>> >>>>>> Initial discussion around the issue can be seen here: >>>>>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-March/032348.html >>>>>> >>>>>> >>>>>> Question for reviewers: I will log a CCC for this - but I am wondering >>>>>> whether I should plan to backport the fix to earlier release? >>>>>> >>>>>> >>>>>> best regards, >>>>>> >>>>>> -- daniel >>>>> >>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif> >>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif> >>>>> >>>>> <http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance >>>>> Andersen| >>>>> Principal Member of Technical Staff | +1.781.442.2037 >>>>> Oracle Java Engineering >>>>> 1 Network Drive >>>>> Burlington, MA 01803 >>>>> lance.ander...@oracle.com <mailto:lance.ander...@oracle.com> >>>>> >>>>> >>>>> >>>> >> >
