[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-gc-dev
Subject:    Re: RFR(S): 8164028: Convert TestPredictions_test to GTest
From:       Kirill Zhaldybin <kirill.zhaldybin () oracle ! com>
Date:       2016-08-22 15:58:20
Message-ID: cbab06d8-2db7-39fb-fab6-9929dda1d85c () oracle ! com
[Download RAW message or body]

Erik,

Thank you for review!

Regards, Kirill

On 22.08.2016 18:13, Erik Helin wrote:
> On 2016-08-22, Kirill Zhaldybin wrote:
> > Erik,
> > 
> > I deleted some comments, please do not hesitate me if you think I deleted
> > too much.Here are a new WebRev:
> > > > http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164028/webrev.04/
> > > > 
> > > > Could you please read comments inline?
> > > > - Looking at the unified diff (much easier now, thanks) it seems like
> > > > the old code used <, which means that the new code must use ASSERT_GE,
> > > > not ASSERT_GT.
> > > > Sorry, I did not find a line you mentioned. Could you please let me know the
> > > > line number?
> > > Just look at the unified diff:
> > > http://cr.openjdk.java.net/~kzhaldyb/webrevs/JDK-8164028/webrev.04/test/native/gc/g1/test_g1Predictions.cpp.udiff.html
> > >  
> > > Everywhere you see a pattern similar to:
> > > -  assert(p2 < p1, "First prediction must be larger than second, but they are \
> > > %f %f", p1, p2); +  ASSERT_GT(p1, p2) << "First prediction must be greater than \
> > > second"; 
> > > you need to change ASSERT_GT to ASSERT_GE.
> > In my understanding assert (p2<p1) means the same as ASSERT_GT(p1, p2)
> > (p1>p2 <=> p2<p1)
> > Could you please correct me if I am wrong?
> No, you are right. Sorry, my bad. Looks good, Reviewed.
> 
> Thanks,
> Erik
> 
> > Thank you.
> > 
> > Regards, Kirill
> > > Thanks,
> > > Eirk
> > > 


[prev in list] [next in list] [prev in thread] [next in thread]