<div dir="ltr">You're right for the single threaded part; I misread that part and thought it would be multi-threaded as well. And fair enough for the keeping it then as a do..while(false); it just took me a while to figure out what was being done. You could put the data.lastLine in a local variable and update it at the start of the method (only using the local version for the rest of the method); then everything would be in there. But, I'll still say it is a more a question of style :)<div><br></div><div>LGTM,</div><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Fri, Oct 5, 2018 at 12:01 PM 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>
<br>
On 10/05/2018 10:34, JC Beyler wrote:<br>
> Hi Alex,<br>
> <br>
> One question and a comment on this:<br>
> - I thought HashMap was not thread safe so I think you need to <br>
> synchronize the access to the map threadData<br>
<br>
The map is accessed from a single thread (main test thread which sends <br>
jdb commands and handles jdb replies), so synchronization is not required.<br>
<br>
> <br>
> - I think your test code could be simplified if you moved it into a <br>
> helper method (not tested but just for example):<br>
<br>
I suppose you don't like do/break/while(false)?<br>
To me it's quite standard method to avoid multi-level if/then/else.<br>
In your suggestion I don't like that processNewData() method handles <br>
minLine/maxLine, but doesn't handle lastLine (i.e. it doesn't do all <br>
processing). But if "data.lastLine = lineNum" is moved into the method, <br>
we need something like do/break/while(false) in the method.<br>
<br>
--alex<br>
<br>
> <br>
> +    private void next() {<br>
> +        List<String> reply = jdb.command(JdbCommand.next());<br>
> +        /*<br>
> +         * Each "next" produces something like ("Breakpoint hit" line <br>
> only if the line has BP)<br>
> +         *   Step completed:<br>
> +         *     Breakpoint hit: "thread=jj2", <br>
> DeferredStepTestTarg$jj2.run(), line=74 bci=12<br>
> +         *     74                    ++count2;                          <br>
>   // @2 breakpoint<br>
> +         *     <empty line><br>
> +         *     jj2[1]<br>
> +         */<br>
> +        // detect thread from the last line<br>
> +        String lastLine = reply.get(reply.size() - 1);<br>
> +        String threadName = parse(threadRegexp, lastLine);<br>
> +        String wholeReply = <br>
> reply.stream().collect(Collectors.joining(Utils.NEW_LINE));<br>
> +        int lineNum = Integer.parseInt(parse(lineRegexp, wholeReply));<br>
> +<br>
> +        System.out.println("got: thread=" + threadName + ", line=" + <br>
> lineNum);<br>
> +<br>
> +        ThreadData data = threadData.get(threadName);<br>
> +        if (data == null) {<br>
> +            data = new ThreadData();<br>
> +            threadData.put(threadName, data);<br>
> +        }<br>
> +        processNewData(data, threadName, lineNum);<br>
> +        data.lastLine = lineNum;<br>
> +    }<br>
> +<br>
> +  private void processNewData(ThreadData data, String threadName, int <br>
> lineNum) {<br>
> +        if (data.lastLine < 0) {<br>
> +            // the 1st stop in the thread<br>
> +            return;<br>
> +        }<br>
> +<br>
> +        if (lineNum == data.lastLine + 1) {<br>
> +            // expected.<br>
> +            return;<br>
> +        }<br>
> +<br>
> +        if (lineNum < data.lastLine) {<br>
> +            // looks like step to the beginning of the cycle<br>
> +            if (data.minLine > 0) {<br>
> +               // minLine and maxLine are not set - verify<br>
> +               Asserts.assertEquals(lineNum, data.minLine, threadName + <br>
> " - minLine");<br>
> +               Asserts.assertEquals(data.lastLine, data.maxLine, <br>
> threadName + " - maxLine");<br>
> +            } else {<br>
> +                // set minLine/maxLine<br>
> +                data.minLine = lineNum;<br>
> +                data.maxLine = data.lastLine;<br>
> +           }<br>
> +           return;<br>
> +        }<br>
> +<br>
> +        throw new RuntimeException(threadName + " (line " + lineNum + <br>
> ") - unexpected."<br>
> +            + " lastLine=" + data.lastLine + ", minLine=" + <br>
> data.minLine + ", maxLine=" + data.maxLine);<br>
> + }<br>
> <br>
> Thanks,<br>
> Jc<br>
> <br>
> <br>
> <br>
> On Thu, Oct 4, 2018 at 6:31 PM <<a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a> <br>
> <mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a>>> wrote:<br>
> <br>
>     Hi Alex,<br>
> <br>
>     It looks good to me.<br>
>     Could you, please, also remove the line? :<br>
> <br>
>        156             //<br>
> <br>
>     No need in new webrev.<br>
> <br>
>     Thanks,<br>
>     Serguei<br>
> <br>
> <br>
>     On 10/4/18 4:11 PM, Alex Menkov wrote:<br>
>      > Hi Serguei,<br>
>      ><br>
>      > Updated webrev:<br>
>      ><br>
>     <a href="http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev.02/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev.02/</a><br>
>     <<a href="http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev.02/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev.02/</a>><br>
>      ><br>
>      > Fixed all issues except<br>
>      >  140                 // the 1st stop in the thread<br>
>      >  141                 break;<br>
>      > In this case the comment is an explanation why we reach the<br>
>     block, not<br>
>      > an explanation for the "break" statement.<br>
>      ><br>
>      > --alex<br>
>      ><br>
>      > On 10/04/2018 13:56, <a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a><br>
>     <mailto:<a href="mailto:serguei.spitsyn@oracle.com" target="_blank">serguei.spitsyn@oracle.com</a>> wrote:<br>
>      >> Hi Alex,<br>
>      >><br>
>      >> Several minor suggestions.<br>
>      >><br>
>      >> 77 new Thread(aRP, "jj1").start();<br>
>      >> 78 new Thread(asRP, "jj2").start(); What mean aRP and asRP? In<br>
>     fact,<br>
>      >> it is confusing. Can they be renamed to something like obj1 and<br>
>     obj2.<br>
>      >><br>
>      >> 79 // new Thread(aRP, "jj3").start();<br>
>      >> 80 // new Thread(asRP, "jj4").start();<br>
>      >><br>
>      >>   These lines can be removed.<br>
>      >><br>
>      >> 94 // line of the last stop<br>
>      >> 95 int lastLine = -1;<br>
>      >> 96 // min line (-1 means "not known yet")<br>
>      >> 97 int minLine = -1;<br>
>      >> 98 // max line (-1 means "not known yet")<br>
>      >> 99 int maxLine = -1; ... 140 // the 1st stop in the thread<br>
>      >> 141 break;<br>
>      >><br>
>      >>    I'd suggest the refactor above as below:<br>
>      >><br>
>      >> int lastLine = -1;  // line of the last stop<br>
>      >> int minLine = -1;  // min line (-1 means "not known yet")<br>
>      >> int maxLine = -1;// max line (-1 means "not known yet")<br>
>      >>   ...<br>
>      >> break;  // the 1st stop in the thread<br>
>      >><br>
>      >> 116 private void next() {<br>
>      >> 117 List<String> reply = jdb.command(JdbCommand.next());<br>
>      >> 118 /* each "next" produces something like ("Breakpoint hit" line<br>
>      >> only if the line has BP)<br>
>      >> 119 Step completed:<br>
>      >> 120 Breakpoint hit: "thread=jj2", DeferredStepTestTarg$jj2.run(),<br>
>      >> line=74 bci=12<br>
>      >> 121 74 ++count2; // @2 breakpoint<br>
>      >> 122 <empty line><br>
>      >> 123 jj2[1]<br>
>      >> 124 */ It would better to have it in this style: 118 /* * Each<br>
>     "next"<br>
>      >> produces something like ("Breakpoint hit" line only if the line<br>
>     has BP).<br>
>      >> 119 * Step completed:<br>
>      >> 120 * Breakpoint hit: "thread=jj2", DeferredStepTestTarg$jj2.run(),<br>
>      >> line=74 bci=12<br>
>      >> 121 * 74 ++count2; // @2 breakpoint<br>
>      >> 122 * <empty line><br>
>      >> 123 * jj2[1]<br>
>      >> 124 */<br>
>      >><br>
>      >><br>
>      >> Otherwise, it looks Okay to me.<br>
>      >><br>
>      >><br>
>      >> Thanks,<br>
>      >> Serguei<br>
>      >><br>
>      >> On 10/3/18 5:49 PM, Alex Menkov wrote:<br>
>      >>> Hi all,<br>
>      >>><br>
>      >>> please review a fix for<br>
>      >>> <a href="https://bugs.openjdk.java.net/browse/JDK-8211292" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8211292</a><br>
>      >>> webrev:<br>
>      >>><br>
>     <a href="http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev/</a><br>
>     <<a href="http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev/</a>><br>
>      >>><br>
>      >>> The fix converts manual shell test to automatic java (as Java<br>
>     allows<br>
>      >>> to parse jdb replies much easier).<br>
>      >>> This is the last sub-task for the "shell to java conversion" task,<br>
>      >>> so the fix also removes shared shell scripts.<br>
>      >>><br>
>      >>> --alex<br>
>      >><br>
> <br>
> <br>
> <br>
> -- <br>
> <br>
> Thanks,<br>
> Jc<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>