Hi Jc,
The coding conventions say:
Constant names in upper-case or mixed-case are tolerated, according to
historical necessity.
I didn't find special rule for enums.
The values are converted to string by using Enum.toString, so lowercase
enum values are used to get lowercase strings.
I don't see much sense to use extra toLowerCase();
--alex
On 08/28/2018 15:03, JC Beyler wrote:
Hi Alexey,
I noticed:
http://cr.openjdk.java.net/~amenkov/sh2java/step2/webrev.01/test/jdk/com/sun/jdi/lib/jdb/JdbCommand.java.udiff.html
<http://cr.openjdk.java.net/%7Eamenkov/sh2java/step2/webrev.01/test/jdk/com/sun/jdi/lib/jdb/JdbCommand.java.udiff.html>
- Should the enum types be in lower case? Other exampes in the
vmTestBase and in jdk/com/sun seem to put them in upper case, no?
Apart from that, it looks good to me,
Jc
On Tue, Aug 28, 2018 at 2:03 PM Alex Menkov <alexey.men...@oracle.com
<mailto:alexey.men...@oracle.com>> wrote:
Need one more reviewer for the fix.
--alex
On 08/22/2018 10:18, serguei.spit...@oracle.com
<mailto: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/
<http://cr.openjdk.java.net/%7Eamenkov/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
<mailto: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
<http://cr.openjdk.java.net/%7Eamenkov/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
<http://cr.openjdk.java.net/%7Eamenkov/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
<http://cr.openjdk.java.net/%7Eamenkov/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
<http://cr.openjdk.java.net/%7Eamenkov/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/
<http://cr.openjdk.java.net/%7Eamenkov/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
>>>>
>
--
Thanks,
Jc
