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 :)
LGTM,
Jc
On Fri, Oct 5, 2018 at 12:01 PM Alex Menkov
<alexey.men...@oracle.com <mailto:alexey.men...@oracle.com>> wrote:
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>
> <mailto: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/>
>
<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>
> <mailto: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/>
>
<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
--
Thanks,
Jc
