JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample (original) (raw)

Shafi Ahmad shafi.s.ahmad at oracle.com
Thu Mar 9 05:03:03 UTC 2017

Hi David, Coleen,

Thank you for the review. I will send the updated webrev link.

Regards, Shafi

-----Original Message----- From: David Holmes Sent: Thursday, March 09, 2017 7:35 AM To: Coleen Phillimore <coleen.phillimore at oracle.com>; hotspot- dev at openjdk.java.net Subject: Re: JDK 10 RFR JDK-8167425: Redundant code in method PerfLongVariant::sample

Hi Coleen, On 9/03/2017 1:42 AM, coleen.phillimore at oracle.com wrote: > > This doesn't look right. > > Don't you need to still check for sampled != NULL? That use of sampled was in dead code that is now removed. if (samplehelper == NULL) return; - if (samplehelper != NULL) { (jlong)valuep = samplehelper->takesample(); - } - else if (sampled != NULL) { - (jlong)valuep = *sampled; - } } This change looks fine to me. > Can you remove the JJJ in the comment? I second that :) Thanks, David ----- > I don't know what tests were run > where Jon (aka JJJ in the code) found this, do you? > > Thanks, > Coleen > > > On 3/8/17 10:21 AM, Shafi Ahmad wrote: >> Hi, >> >> May I get the review done for this. >> >> Regards, >> Shafi >> >>> -----Original Message----- >>> From: Shafi Ahmad >>> Sent: Wednesday, March 01, 2017 4:27 PM >>> To: hotspot-dev at openjdk.java.net >>> Subject: FW: JDK 10 RFR JDK-8167425: Redundant code in method >>> PerfLongVariant::sample >>> >>> Hi, >>> >>> Summary: >>> It's a very small change to a single file. >>> >>> void PerfLongVariant::sample() { >>> >>> - assert(samplehelper != NULL || sampled != NULL, "unexpected >>> state"); >>> + // JJJ - This should not happen. Maybe the first sample is taken >>> + // while the samplehelper is being null'ed out. >>> + // assert(samplehelper != NULL || sampled != NULL, "unexpected >>> + state"); if (samplehelper == NULL) return; >>> >>> if (samplehelper != NULL) { >>> (jlong)valuep = samplehelper->takesample(); >>> } >>> else if (sampled != NULL) { >>> (jlong)valuep = *sampled; >>> } >>> } >>> Above five lines are modified in changeset >>> http://hg.openjdk.java.net/jdk8/jdk8/hotspot/rev/da91efe96a93#l802.1 >>> 2 Due to the addition of NULL check and return >>> if (samplehelper == NULL) return; the else-if block becomes >>> redundant and statement 'if (samplehelper != NULL)' is not needed. >>> >>> Webrev link: http://cr.openjdk.java.net/~shshahma/8167425/webrev.00/ >>> Jdk10 bug link: https://bugs.openjdk.java.net/browse/JDK-8167425 >>> >>> Testing: jprt. >>> >>> Thanks, >>> Shafi >

More information about the hotspot-dev mailing list