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>
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>> 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
>
--
Thanks,
Jc
