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