Hi Jc,

On 10/05/2018 10:34, JC Beyler wrote:
Hi Alex,

One question and a comment on this:
- I thought HashMap was not thread safe so I think you need to synchronize the access to the map threadData

The map is accessed from a single thread (main test thread which sends jdb commands and handles jdb replies), so synchronization is not required.


- I think your test code could be simplified if you moved it into a helper method (not tested but just for example):

I suppose you don't like do/break/while(false)?
To me it's quite standard method to avoid multi-level if/then/else.
In your suggestion I don't like that processNewData() method handles minLine/maxLine, but doesn't handle lastLine (i.e. it doesn't do all processing). But if "data.lastLine = lineNum" is moved into the method, we need something like do/break/while(false) in the method.

--alex


+    private void next() {
+        List<String> reply = jdb.command(JdbCommand.next());
+        /*
+         * Each "next" produces something like ("Breakpoint hit" line only if the line has BP)
+         *   Step completed:
+         *     Breakpoint hit: "thread=jj2", DeferredStepTestTarg$jj2.run(), line=74 bci=12 +         *     74                    ++count2;  // @2 breakpoint
+         *     <empty line>
+         *     jj2[1]
+         */
+        // detect thread from the last line
+        String lastLine = reply.get(reply.size() - 1);
+        String threadName = parse(threadRegexp, lastLine);
+        String wholeReply = reply.stream().collect(Collectors.joining(Utils.NEW_LINE));
+        int lineNum = Integer.parseInt(parse(lineRegexp, wholeReply));
+
+        System.out.println("got: thread=" + threadName + ", line=" + lineNum);
+
+        ThreadData data = threadData.get(threadName);
+        if (data == null) {
+            data = new ThreadData();
+            threadData.put(threadName, data);
+        }
+        processNewData(data, threadName, lineNum);
+        data.lastLine = lineNum;
+    }
+
+  private void processNewData(ThreadData data, String threadName, int lineNum) {
+        if (data.lastLine < 0) {
+            // the 1st stop in the thread
+            return;
+        }
+
+        if (lineNum == data.lastLine + 1) {
+            // expected.
+            return;
+        }
+
+        if (lineNum < data.lastLine) {
+            // looks like step to the beginning of the cycle
+            if (data.minLine > 0) {
+               // minLine and maxLine are not set - verify
+               Asserts.assertEquals(lineNum, data.minLine, threadName + " - minLine"); +               Asserts.assertEquals(data.lastLine, data.maxLine, threadName + " - maxLine");
+            } else {
+                // set minLine/maxLine
+                data.minLine = lineNum;
+                data.maxLine = data.lastLine;
+           }
+           return;
+        }
+
+        throw new RuntimeException(threadName + " (line " + lineNum + ") - unexpected." +            + " lastLine=" + data.lastLine + ", minLine=" + data.minLine + ", maxLine=" + data.maxLine);
+ }

Thanks,
Jc



On Thu, Oct 4, 2018 at 6:31 PM <serguei.spit...@oracle.com <mailto:serguei.spit...@oracle.com>> wrote:

    Hi Alex,

    It looks good to me.
    Could you, please, also remove the line? :

       156             //

    No need in new webrev.

    Thanks,
    Serguei


    On 10/4/18 4:11 PM, Alex Menkov wrote:
     > Hi Serguei,
     >
     > Updated webrev:
     >
    http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev.02/
    
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev.02/>
     >
     > Fixed all issues except
     >  140                 // the 1st stop in the thread
     >  141                 break;
     > In this case the comment is an explanation why we reach the
    block, not
     > an explanation for the "break" statement.
     >
     > --alex
     >
     > On 10/04/2018 13:56, serguei.spit...@oracle.com
    <mailto:serguei.spit...@oracle.com> wrote:
     >> Hi Alex,
     >>
     >> Several minor suggestions.
     >>
     >> 77 new Thread(aRP, "jj1").start();
     >> 78 new Thread(asRP, "jj2").start(); What mean aRP and asRP? In
    fact,
     >> it is confusing. Can they be renamed to something like obj1 and
    obj2.
     >>
     >> 79 // new Thread(aRP, "jj3").start();
     >> 80 // new Thread(asRP, "jj4").start();
     >>
     >>   These lines can be removed.
     >>
     >> 94 // line of the last stop
     >> 95 int lastLine = -1;
     >> 96 // min line (-1 means "not known yet")
     >> 97 int minLine = -1;
     >> 98 // max line (-1 means "not known yet")
     >> 99 int maxLine = -1; ... 140 // the 1st stop in the thread
     >> 141 break;
     >>
     >>    I'd suggest the refactor above as below:
     >>
     >> int lastLine = -1;  // line of the last stop
     >> int minLine = -1;  // min line (-1 means "not known yet")
     >> int maxLine = -1;// max line (-1 means "not known yet")
     >>   ...
     >> break;  // the 1st stop in the thread
     >>
     >> 116 private void next() {
     >> 117 List<String> reply = jdb.command(JdbCommand.next());
     >> 118 /* each "next" produces something like ("Breakpoint hit" line
     >> only if the line has BP)
     >> 119 Step completed:
     >> 120 Breakpoint hit: "thread=jj2", DeferredStepTestTarg$jj2.run(),
     >> line=74 bci=12
     >> 121 74 ++count2; // @2 breakpoint
     >> 122 <empty line>
     >> 123 jj2[1]
     >> 124 */ It would better to have it in this style: 118 /* * Each
    "next"
     >> produces something like ("Breakpoint hit" line only if the line
    has BP).
     >> 119 * Step completed:
     >> 120 * Breakpoint hit: "thread=jj2", DeferredStepTestTarg$jj2.run(),
     >> line=74 bci=12
     >> 121 * 74 ++count2; // @2 breakpoint
     >> 122 * <empty line>
     >> 123 * jj2[1]
     >> 124 */
     >>
     >>
     >> Otherwise, it looks Okay to me.
     >>
     >>
     >> Thanks,
     >> Serguei
     >>
     >> On 10/3/18 5:49 PM, Alex Menkov wrote:
     >>> Hi all,
     >>>
     >>> please review a fix for
     >>> https://bugs.openjdk.java.net/browse/JDK-8211292
     >>> webrev:
     >>>
    http://cr.openjdk.java.net/~amenkov/sh2java/DeferredStep_final/webrev/
    <http://cr.openjdk.java.net/%7Eamenkov/sh2java/DeferredStep_final/webrev/>
     >>>
     >>> The fix converts manual shell test to automatic java (as Java
    allows
     >>> to parse jdb replies much easier).
     >>> This is the last sub-task for the "shell to java conversion" task,
     >>> so the fix also removes shared shell scripts.
     >>>
     >>> --alex
     >>



--

Thanks,
Jc

Reply via email to