Hello All, I need one more review. Please review.
Thanks, Jay -----Original Message----- From: Vadim Pakhnushev Sent: Tuesday, October 20, 2015 11:44 AM To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip Race Subject: Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing +1 On 20.10.2015 8:31, Jayathirth D V wrote: > Hi Vadim, > > Thanks for throwing light on performance aspect of Boxing & Unboxing in Java. > > I have made changes, so that we use Float.compare and then use equality > operator to determine whether expected & returned values are same. > > Please find updated Webrev: > > http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.07/ > > Please review. > > Thanks, > Jay > > -----Original Message----- > From: Vadim Pakhnushev > Sent: Monday, October 19, 2015 4:50 PM > To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip > Race > Subject: Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for > JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing > > Jay, > What I mean is that you can either declare two floats (lowercase) and then > you need to do if (Float.compare(f1, f2) == 0) Or you declare a Float f1 and > then you can write if (f1.equals(f2)) In the first case there isn't any > boxing, while in the second case second float will be boxed (and unboxed in > the equals method). > So technically Float.compare(f1, f2) == 0 is the most efficient way to > compare two primitive floats, especially given that Float.parseFloat returns > primitive float. > In this particular case simple == would be sufficient though, since one of > the floats is computed at compile time and you know that you won't be > comparing NaN's and expecting that they are equal... > > I'm OK with both approaches, but would prefer primitive types and > (Float.compare(f1, f2) == 0). > > Thanks, > Vadim > > On 19.10.2015 14:04, Jayathirth D V wrote: >> Hi Vadim, >> >> I think doing compare and then equals, actually increases the computation we >> are doing to check whether expected value and returned value are same. >> >> New approach of directly using equals to compare between expected and >> returned value is efficient. >> >> I have made changes you mentioned regarding the typo in "spacing". Please >> find updated Webrev : >> >> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.06/ >> >> Please review so that we can push the change. >> >> Thanks, >> Jay >> >> -----Original Message----- >> From: Vadim Pakhnushev >> Sent: Monday, October 19, 2015 4:03 PM >> To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip >> Race >> Subject: Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for >> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing >> >> Hi Jay, >> >> I'm sorry, actually the usage of Float.compare was perfectly fine in your >> case, given that you were comparing floats (not Floats). >> The thing which caught my eye was the use of Integer boxing as Sergey >> pointed out. >> Sorry about the confusion. >> >> Thanks, >> Vadim >> >> On 19.10.2015 12:04, Jayathirth D V wrote: >>> Hi Vadim, >>> >>> Thanks for the review. >>> I have made suggested changes. Updated Webrev : >>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.05/ >>> >>> Please review. >>> >>> Thanks, >>> Jay >>> >>> -----Original Message----- >>> From: Vadim Pakhnushev >>> Sent: Friday, October 16, 2015 8:15 PM >>> To: Jayathirth D V; Sergey Bylokhov; 2d-dev@openjdk.java.net; Philip >>> Race >>> Subject: Re: [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for >>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing >>> >>> Hi Jay, >>> >>> What's the point of using Float.compare in the test? >>> Why not just check if >>> (horizontalNodeValue.equals(expectedHorizontalValue)) ? >>> Also please capitalize Attr in the declaration of horizontalattr and >>> verticalattr. >>> >>> Thanks, >>> Vadim >>> >>> On 16.10.2015 17:36, Jayathirth D V wrote: >>>> Hello All, >>>> >>>> Can I get one more review please. >>>> >>>> Thanks, >>>> Jay >>>> >>>> -----Original Message----- >>>> From: Sergey Bylokhov >>>> Sent: Thursday, October 15, 2015 6:05 PM >>>> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race >>>> Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-7182758: >>>> BMPMetadata returns invalid PhysicalPixelSpacing >>>> >>>> The fix looks fine. The test can be improved a little bit to simplify the >>>> int->Integer boxing, but it is not necessary for now. >>>> >>>> Thanks. >>>> >>>> On 15.10.15 13:17, Jayathirth D V wrote: >>>>> Hi Sergey, >>>>> >>>>> I thought you suggested to check for tighter "true" condition instead of >>>>> "false" condition. >>>>> >>>>> I have made changes to map horizontalNodeValue and verticalNodeValue to >>>>> expected values. Please find updated Webrev link: >>>>> >>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.04/ >>>>> >>>>> Please review. >>>>> >>>>> Thanks, >>>>> Jay >>>>> >>>>> -----Original Message----- >>>>> From: Sergey Bylokhov >>>>> Sent: Wednesday, October 14, 2015 7:34 PM >>>>> To: Jayathirth D V; 2d-dev@openjdk.java.net; Philip Race >>>>> Subject: Re: <OpenJDK 2D-Dev> Review request for JDK-7182758: >>>>> BMPMetadata returns invalid PhysicalPixelSpacing >>>>> >>>>> Hi, Jay. >>>>>> I suggest to check correct/expected result in the test instead of >>>>>> previous incorrect zero. >>>>> Here, I suggested to check that the resulted horizontalNodeValue and >>>>> verticalNodeValue are equal to some expected value. Because if it bigger >>>>> than zero does not mean it is correct(note to use Float.compare(f1, f2) >>>>> instead of "=="). >>>>> >>>>>> Previously I forgot to mention, that it will be good to name the test by >>>>>> some useful name instead of some bug number(see examples in >>>>>> javax/imageio/plugins). >>>>>> >>>>>> On 13.10.15 11:12, Jayathirth D V wrote: >>>>>>> Hello All, >>>>>>> >>>>>>> Removed Trailing whitespace present in code. >>>>>>> >>>>>>> Please find updated webrev link: >>>>>>> >>>>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.02/ >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Jay >>>>>>> >>>>>>> *From:* Jayathirth D V >>>>>>> *Sent:* Monday, October 12, 2015 2:15 PM >>>>>>> *To:* 2d-dev@openjdk.java.net; Philip Race; Sergey Bylokhov >>>>>>> *Subject:* [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for >>>>>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing >>>>>>> >>>>>>> Hello All, >>>>>>> >>>>>>> Made small change on how we need to represent floating point >>>>>>> constant in >>>>>>> Java(1000.0 -> 1000.0F). >>>>>>> >>>>>>> Please find updated Webrev link: >>>>>>> >>>>>>> Webrev : >>>>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.01/ >>>>>>> >>>>>>> Please review. >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Jay >>>>>>> >>>>>>> *From:* Jayathirth D V >>>>>>> *Sent:* Thursday, October 08, 2015 2:20 PM >>>>>>> *To:* 2d-dev@openjdk.java.net <mailto:2d-dev@openjdk.java.net>; >>>>>>> Philip Race; Sergey Bylokhov >>>>>>> *Subject:* [OpenJDK 2D-Dev] <OpenJDK 2D-Dev> Review request for >>>>>>> JDK-7182758: BMPMetadata returns invalid PhysicalPixelSpacing >>>>>>> >>>>>>> Hello All, >>>>>>> >>>>>>> Please review following fix in jdk9: >>>>>>> >>>>>>> Bug : https://bugs.openjdk.java.net/browse/JDK-7182758 >>>>>>> >>>>>>> Webrev : >>>>>>> http://cr.openjdk.java.net/~rchamyal/jay/7182758/webrev.00/ >>>>>>> >>>>>>> Bug : BMPMetadata returns invalid PhysicalPixelSpacing >>>>>>> >>>>>>> Root cause : Whenever XPixelsPerMter or YPixelsPerMeter is more >>>>>>> than value 1 in BMP header. Horizontal & Vertical Physical pixel >>>>>>> spacing were returned as zero. >>>>>>> >>>>>>> In getStandardDimensionNode() >>>>>>> method of BMPMetadata.java we are dividing 1 by XPixelsPerMter/ >>>>>>> YPixelsPerMter. >>>>>>> When >>>>>>> >>>>>>> XPixelsPerMter/ YPixelsPerMter >>>>>>> is more than 1. Resulted value is stored without decimal part, which >>>>>>> resulted in zero. >>>>>>> >>>>>>> Solution : Made changes to how Horizontal & Vertical Physical >>>>>>> pixel spacing is calculated so that decimal value is not truncated. >>>>>>> >>>>>>> Thanks, >>>>>>> >>>>>>> Jay >>>>>>> >>>>>> -- >>>>>> Best regards, Sergey. >>>>>> >>>>> -- >>>>> Best regards, Sergey. >>>>> >>>> -- >>>> Best regards, Sergey.
