<div dir="ltr">Hi Alex,<div><br></div><div>Looks great to me, thanks for cleaning it up!</div><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Oct 11, 2018 at 11:02 AM Alex Menkov <<a href="mailto:alexey.menkov@oracle.com">alexey.menkov@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Jc,<br>
<br>
updated webrev:<br>
<a href="http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.02/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.02/</a><br>
<br>
> <<a href="http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html</a>><br>
>>> -> Why not make it javadoc like the other methods of the same file<br>
>>> (so @return instead of returns and a second * at the start of the<br>
>>     comment)?<br>
<br>
done.<br>
<br>
<br>
>>> b) Nit: Is there a reason we are complicating the code here:<br>
>>><br>
>>>           try {<br>
>>>               LingeredApp a = LingeredApp.startApp(cmd);<br>
>>> +<br>
>>> + // startApp is expected to fail, but if not, terminate the app<br>
>>> + try {<br>
>>> + a.stopApp();<br>
>>> + } catch (IOException e) {<br>
>>> + // print and let the test fail<br>
>>> + System.err.println("LingeredApp.stopApp failed");<br>
>>> + e.printStackTrace();<br>
>>> + }<br>
>>>           } catch (IOException ex) {<br>
>>>               System.err.println(testName + ": caught expected<br>
>>     IOException");<br>
>>>               System.err.println(testName + " PASSED");<br>
>>>               return;<br>
>>>           }<br>
>>><br>
>>> Why not just put it below? We could either put a outside the try<br>
>>     and then move that code out; or perhaps move it into a separate<br>
>>     method to let<br>
>>><br>
>>> the reader concentrate on the test at hand and let the "stopping<br>
>>     of the app"  happen somewhere else?<br>
<br>
This is to improve code cleanup on error (the test expects that <br>
LingeredApp.startApp(cmd) fails, but if the test fails, the app remains <br>
running).<br>
I moved cleanup outside of the try (and removed try/catch around <br>
a.stopApp() - the test throws Exception anyway)<br>
<br>
--alex<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>