On 11/10/2018 1:09 PM, JC Beyler wrote:
So that's what I thought too but I did this:$ cat Test.java public class Test { public static void main(String[] args) { int res; try { res = 5; } catch (Exception e) { } if (res > 0) { } } } That fails to compile with JDK8 and JDK11 it seems. Perhaps I'm doing something wrong?
Yes. :) You are catching the theoretical exception from the assignment and so allowing control to reach the if statement - at which point res may not be initialized.
David -----
$ javac Test.java Test.java:10: error: variable res might not have been initialized if (res > 0) { ^ 1 error Sorry if I'm derailing the review :) JcOn Wed, Oct 10, 2018 at 7:36 PM David Holmes <[email protected] <mailto:[email protected]>> wrote:On 11/10/2018 12:19 PM, JC Beyler wrote: > Hi Alex, > > - > http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html <http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html> > <http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/lib/jdk/test/lib/apps/LingeredApp.java.udiff.html> > -> Why not make it javadoc like the other methods of the same file > (so @return instead of returns and a second * at the start of the comment)? > > - > http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html <http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html> > <http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/test/jdk/com/sun/jdi/BasicJDWPConnectionTest.java.udiff.html> > a) I'm surprised by this: > > + int res; > + try { > + res = handshake(detectPort(a.getProcessStdout())); > + } finally { > a.stopApp(); > + } > if (res < 0) { > > > I would have thought that this makes javac return a "res might not be initialized" error. res can only be uninitialized if an exception occurs, in which case you won't reach the "if (res , 0)" statement. David ----- > > b) Nit: Is there a reason we are complicating the code here: > > try { > LingeredApp a = LingeredApp.startApp(cmd); > + > + // startApp is expected to fail, but if not, terminate the app > + try { > + a.stopApp(); > + } catch (IOException e) { > + // print and let the test fail > + System.err.println("LingeredApp.stopApp failed"); > + e.printStackTrace(); > + } > } catch (IOException ex) { > System.err.println(testName + ": caught expected IOException"); > System.err.println(testName + " PASSED"); > return; > } > > Why not just put it below? We could either put a outside the try and then move that code out; or perhaps move it into a separate method to let > > the reader concentrate on the test at hand and let the "stopping of the app" happen somewhere else? > > > Thanks, > Jc > > On Wed, Oct 10, 2018 at 5:18 PM [email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>> <[email protected] <mailto:[email protected]> > <mailto:[email protected] <mailto:[email protected]>>> wrote: > > Hi Alex, > > It looks good to me. > How did you test it? > > Thanks, > Serguei > > > On 10/10/18 16:25, Alex Menkov wrote: > > Hi all, > > > > please review a fix for > > https://bugs.openjdk.java.net/browse/JDK-8195703 > > webrev: > > http://cr.openjdk.java.net/~amenkov/BasicJDWPConn/webrev.01/ <http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/> > <http://cr.openjdk.java.net/%7Eamenkov/BasicJDWPConn/webrev.01/> > > > > I was not able to reproduce the issue, but accordingly the logs in > > jira root cause is a transport initialization error "Address already > > in use". > > The test uses Utils.getFreePort() to select some free port, but > it can > > be race condition when some other app (or other test) uses the port > > selected before debuggee application starts to listen on it. > > The fix uses dynamic port allocation and then parses it from the > > debuggee output. > > Other changes: > > - dropped catching exceptions and calling System.exit() - this > causes > > SecurityException in JTReg harness which makes error analysis much > > harder; > > - dropped using of Utils.getFreePort() from jdi/DoubleAgentTest.java > > test; > > jdi/BadHandshakeTest.java also uses Utils.getFreePort(), but it > > handles "Already in use" error re-peeking other free port and > > restarting debuggee, so I keep it as is. > > > > --alex > > > > > > -- > > Thanks, > Jc -- Thanks, Jc
