<div dir="ltr">Hi Igor,<div><br></div><div>Looks good to me as well, I agree with Thomas' comments and also prefer RAII, especially since SetUp/TearDown here is going to be weird since you don't always create the object the same way.</div><div><br></div><div>If you make a good RAII object, then both tests could use the same kind of code instead of one using a helper method and another creating the ReservedSpace directly too.</div><div><br></div><div>Thanks!</div><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 17, 2018 at 11:54 AM Thomas Stüfe <<a href="mailto:thomas.stuefe@gmail.com">thomas.stuefe@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, Oct 17, 2018 at 8:29 PM Igor Ignatyev <<a href="mailto:igor.ignatyev@oracle.com" target="_blank">igor.ignatyev@oracle.com</a>> wrote:<br>
><br>
> Hi Thomas,<br>
><br>
> 1st of all, thanks for looking at it, appreciate that.<br>
><br>
> it seems we can. there are two ways I can fix that either replace ASSERT by EXCEPT or introduce a text fixture w/ SetUp and TearDown. which do you prefer?<br>
><br>
> -- Igor<br>
<br>
Slight preference for SetUp/TearDown (EXCEPT would quit the testing<br>
altogether, wont it?).<br>
<br>
Lazy me would have just used an RAII object like this:<br>
<br>
struct Cleaner {<br>
  ReservedSpace* rs;<br>
  ~Cleaner() { rs.release(); }<br>
};<br>
<br>
But I leave it up to you.<br>
<br>
... Thomas<br>
<br>
><br>
> > On Oct 17, 2018, at 11:16 AM, Thomas Stüfe <<a href="mailto:thomas.stuefe@gmail.com" target="_blank">thomas.stuefe@gmail.com</a>> wrote:<br>
> ><br>
> > Hi Igor,<br>
> ><br>
> > test_virtual_space_actual_committed_space() and<br>
> > actual_committed_space_one_large_page():<br>
> ><br>
> > Won't we leak the reserved space now when we run into an ASSERT and<br>
> > then continue with the next gtests?<br>
> ><br>
> > Cheers, Thomas<br>
> ><br>
> > On Wed, Oct 17, 2018 at 7:55 PM Igor Ignatyev <<a href="mailto:igor.ignatyev@oracle.com" target="_blank">igor.ignatyev@oracle.com</a>> wrote:<br>
> >><br>
> >> <a href="http://cr.openjdk.java.net/~iignatyev//8177709/webrev.00/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~iignatyev//8177709/webrev.00/index.html</a><br>
> >>> 141 lines changed: 140 ins; 1 del; 0 mod;<br>
> >><br>
> >> Hi all,<br>
> >><br>
> >> could you please review this small (and hopefully trivial) patch which converts internal TestVirtualSpace_test to gtest?<br>
> >> again, the old test is still used by WhiteBox::runMemoryUnitTests, the old code hasn't been removed. it will be removed later when all 4 tests used by runMemoryUnitTests are converted.<br>
> >><br>
> >> for the sake of reviewer, <a href="http://hg.openjdk.java.net/jdk/jdk/file/2e928420389d/src/hotspot/share/memory/virtualspace.cpp#l1261" rel="noreferrer" target="_blank">http://hg.openjdk.java.net/jdk/jdk/file/2e928420389d/src/hotspot/share/memory/virtualspace.cpp#l1261</a> is the old version.<br>
> >><br>
> >> webrev: <a href="http://cr.openjdk.java.net/~iignatyev//8177709/webrev.00/index.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~iignatyev//8177709/webrev.00/index.html</a><br>
> >> JBS: <a href="https://bugs.openjdk.java.net/browse/JDK-8177709" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8177709</a><br>
> >> testing:<br>
> >> - converted tests on linux-x64, windows-x64, macosx-x64, solaris-sparcv9 in product and fastdebug variants<br>
> >> - build w/ precompiled-headers enabled and disabled<br>
> >><br>
> >> Thanks,<br>
> >> -- Igor<br>
><br>
</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><br></div>Thanks,<div>Jc</div></div></div>