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

David Holmes david.holmes at oracle.com
Fri Mar 10 11:56:06 UTC 2017

Hi Shafi,

Stylistically this looks very awkward, please rewrite as:

void PerfLongVariant::sample() { if (_sample_helper != NULL) { (jlong)_valuep = _sample_helper->take_sample(); } }

No need for further webrevs.

Thanks, David

On 10/03/2017 9:13 PM, Shafi Ahmad wrote:

Hi,

Thank you Coleen, David and Serguei for the review. Please find updated webrev http://cr.openjdk.java.net/~shshahma/8167425/webrev.02/ Regards, Shafi

-----Original Message----- From: Serguei Spitsyn Sent: Friday, March 10, 2017 4:29 PM To: Shafi Ahmad <shafi.s.ahmad at oracle.com>; David Holmes <david.holmes at oracle.com>; 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 Shafi, I agree the comment is not needed. Otherwise, it looks good to me. Thanks, Serguei

On 3/8/17 21:30, Shafi Ahmad wrote: Hi David, Yes, you are right. The comments are not at all needed because of if (samplehelper == NULL) return; Regards, Shafi -----Original Message----- From: David Holmes Sent: Thursday, March 09, 2017 10:49 AM To: Shafi Ahmad <shafi.s.ahmad at oracle.com>; 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 Shafi, On 9/03/2017 3:08 PM, Shafi Ahmad wrote: Hi,

Please find updated webrev link. http://cr.openjdk.java.net/~shshahma/8167425/webrev.01/ The more I read this code block, the comment and the commented-out assertion: 215 void PerfLongVariant::sample() { 216 217 // This should not happen. Maybe the first sample is taken 218 // while the samplehelper is being null'ed out. 219 // assert(samplehelper != NULL || sampled != NULL, "unexpected state"); 220 if (samplehelper == NULL) return; 221 222 (jlong)valuep = samplehelper->takesample(); 223 } the less it makes sense to me. ??? David

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