Oops. Alex, not Daniil.
Chris
On 8/28/18 11:18 PM, Chris Plummer wrote:
Hi Daniil,
Overall the changes look good. A few cleanup suggestions are below:
 63        setBreakpoints(System.getProperty("test.src") +
"/CatchAllTest.java", 1);
This code is repeated a lot, and all current uses always use test.src.
Can the getProperty part just be moved into setBreakpoints?
 280        final String invalidTypeException =
"InvalidTypeException";
 281
 282 evalShouldContain("EvalArgsTarg.ffboolean(EvalArgsTarg.jjbyte)",
invalidTypeException);
 283
 284 evalShouldContain("EvalArgsTarg.ffintArray(EvalArgsTarg.jjint)",
invalidTypeException);
 285
 286
evalShouldContain("EvalArgsTarg.ffintArray(EvalArgsTarg.jjfloatArray)",
invalidTypeException);
 287
 288 evalShouldContain("EvalArgsTarg.ffjj2(EvalArgsTarg.myjj1)",
invalidTypeException);
 289
 290 evalShouldContain("EvalArgsTarg.ffjj2(EvalArgsTarg.myoranges)",
invalidTypeException);
I think the invalidTypeException variable is unnecessary and just
causes the reader to go find what it is actually set to. Also, line
spacing is not needed here.
 42 * The test makes sure that it is, at all, possible to invoke
an interface
 43 * static method and that the static methods are not inherited
by extending
 44 * interfaces.
I think the "at all" comment should be removed. No idea why it is there.
 40 /*
 41 * This is another test of the same bug. The bug occurs when
trying
 42 * to walk the stack of a deoptimized thread. We can do this
 43 * by running in -Xcomp mode and by doing a step which causes
deopt,
 44 * and then a 'where'. This will cause not all the frames to
be shown.
 45 */
I know you didn't change this comment, but what is "the same bug" in
reference too. Also, is says tested using -Xcomp, but the original
shell script version never seemed to do this. I guess it relied on
-Xcomp testing for the whole test run. You seem to have fixed this in
the java version so the debugee is always run with -Xcomp, which seems
like the right thing to do.
 39 * The bug is that, for example, if a String is passed
 40 * as an arg to a func where an Object is expected,
 41 * the above error occurs. jdb doesnt notice that this is
 42 * legal because String is an instance of Object.
Another unmodified comment that could use some cleaning up. What is
"the above error"? A better description of what is actually being
tested would be useful. Change "doesnt" to "doesn't".
BTW, the navigation buttons at the bottom of your webrev don't seem to
work starting with the 3rd file. The URL gets messed up.
thanks,
Chris
On 8/28/18 5:06 PM, serguei.spit...@oracle.com wrote:
Sorry, forgot to add Chris - fixed now.
Still it'd be good to get a confirmation from Chris.
Thanks,
Serguei
On 8/28/18 16:38, Alex Menkov wrote:
Looks like you sent the message only to me :)
Anyway I've got 2nd review from Jc.
--alex
On 08/28/2018 16:20, serguei.spit...@oracle.com wrote:
Hi Chris,
I guess, Alex is waiting for your final thumbs up.
Alex, please, fix me if it is wrong.
Thanks,
Serguei
On 8/28/18 14:03, Alex Menkov wrote:
Need one more reviewer for the fix.
--alex
On 08/22/2018 10:18, serguei.spit...@oracle.com wrote:
Hi Alex,
This looks good.
Thank you for the update!
Thanks,
Serguei
On 8/21/18 17:10, Alex Menkov wrote:
Hi guys,
Updated webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev.01/
changes (vs initial fix):
- Eval*.java tests are updated, common methods are moved into
JdbTest;
- $classname substitution is dropped in EvalArgs.java;
- comment are reintroduced;
- line separator logic was dropped from JdbCommand (it's not
needed as JdbCommand contains static factory methods to create
commands).
Update of String.split to use "\\R" is done in other fix:
JDK-8209605: com/sun/jdi/BreakpointWithFullGC.java fails with ZGC
--alex
On 08/20/2018 19:12, David Holmes wrote:
Picking up on one point ...
On 21/08/2018 9:40 AM, serguei.spit...@oracle.com wrote:
Hi Alex,
It looks good in general.
Some minor comments below.
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/lib/jdb/JdbCommand.java.frames.html
 132    private static final String ls =
System.getProperty("line.separator");
  Minor: Replace variable name "ls" with "LINE_SEP" (in
upper case as it is a static final).
There have been bugs in the past where tests use the wrong
line-separator because there is confusion as to which output
comes via the OS (and so has platform line-separators) and
which comes more directly (e.g. via socket) and so has the
language line-separator. Are we sure we will always be dealing
with the platform line-separator here?
For String.split use of the regex \\R for the line-separators
seems the best solution.
Thanks,
David
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArgs.java.frames.html
  Just wanted to double-check that this check:
262 jdbFailIfPresent "Arguments match no method"
  works in each call of the verifyNoArgumentsMatchMethod()
  instead of just once as it was originally.
214 private void verifyNoArgumentsMatchMethod(String expr) {
215 List<String> reply =
jdb.command(JdbCommand.eval(expr.replace("$classname",
DEBUGGEE_CLASS)));
216 new
OutputAnalyzer(reply.stream().collect(Collectors.joining(lineSeparator)))
217 .shouldNotContain("Arguments match no method");
218 }
  Minor: It feels like the method name need a tweak.
        Something like: verifyNoArgumentsMatchNoMethod
292 // These overload calls should fail because ther Minor: A
suggestion is to fix a typo at the end of comment.
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalArraysAsList.java.frames.html
  The comment was not converted:
31 # The test checks if evaluation of the expression
java.util.Arrays.asList(null, "a")
32 # works normally and does not throw an
IllegalArgumentException.
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/test/jdk/com/sun/jdi/EvalInterfaceStatic.java.frames.html
  The comment was not converted:
33 # The test exercises the ability to invoke static methods
on interfaces.
34 # Static interface methods are a new feature added in JDK8.
35 #
36 # The test makes sure that it is, at all, possible to
invoke an interface
37 # static method and that the static methods are not
inherited by extending
38 # interfaces.
Thanks,
Serguei
On 8/16/18 14:13, Alex Menkov wrote:
Hi all,
Please review next chunk of shell->java test conversion.
jira: https://bugs.openjdk.java.net/browse/JDK-8209604
webrev:
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev/
The fix contains some changes in library classes:
Jdb.java - timeouts are updated (as per Dan note in one of
previous review, timeouts should respect timeout factor,
Utils.adjustTimeout implements the functionality);
JdbCommand.java - new jdb commands (required by tests) are
added.
--alex
