Looks good! Thanks, /Staffan
On 13 jun 2014, at 14:15, Yekaterina Kantserova <[email protected]> wrote: > Staffan, thanks for your review! > > The new webrev can be found here: > http://cr.openjdk.java.net/~ykantser/6545295/webrev.01/ > > // Katja > > On 06/13/2014 01:44 PM, Staffan Larsen wrote: >> Katja, >> >> Great to see this simplification! A couple of comments: >> >> - I don’t think you need to launch jhat with -XX:+UsePerfData - no one is >> attaching or reading data from the process. >> >> - The static processBuilder field seems to be mis-used. Why not use >> different ProcessBuilder objects for the HelloWorld and jhat? >> >> - For dumpFile.deleteOnExit() to be useful you should add it as early as >> possible (line 48 perhaps?). If it is the last line in the test you can just >> as well do dumpFile.delete(). > > Of cause it should be dumpFile.delete()! - dumpFile.deleteOnExit() has crept > in by mistake. > >> >> /S >> >> >> On 13 jun 2014, at 13:25, Yekaterina Kantserova >> <[email protected]> wrote: >> >>> Hi, >>> >>> Could I please have a review of this fix. >>> >>> webrev: http://cr.openjdk.java.net/~ykantser/6545295 >>> bug: https://bugs.openjdk.java.net/browse/JDK-6545295 >>> >>> The test has been re-written using the testlibrary's functionality. It's >>> verified internally on all basic platforms. >>> >>> Thanks, >>> Katja
