Re: [10] RFR: 8184940: JDK 9 rejects zip files where the modified day or month is 0

[Martin Buchholz]

I took another look.

  90 if ((dtime >> 25) == 0) {

It looks like this will test only the year, not all the date fields.
Shouldn't that be s/25/16/ ?
Does this code handle the "true" epoch of 1980-01-01 ?


On Thu, Jul 27, 2017 at 11:54 AM, Liam Miller-Cushon 
wrote:

> On Wed, Jul 26, 2017 at 4:12 PM, Martin Buchholz 
> wrote:
>
>> I wonder where the "8" in the "Friday the 30th" date comes from.  Could
>> your California time zone be leaking into the test?   The DOS timestamp is
>> interpreted as local time while Instant.parse uses UTC?  Will the test
>> still pass if you run jtreg under TZ=EST?
>>
>
> Thanks, fixed:
> http://cr.openjdk.java.net/~cushon/8184940/webrev.01/
>

Re: RFR(XS) 8184775: tools/launcher/modules/illegalaccess/IllegalAccessTest.java times out on some platforms…

[Martin Buchholz]

OK, that makes sense - short-lived programs with -Xcomp are dominated by
cost of compiling everything.

On Fri, Jul 28, 2017 at 2:30 PM, Leonid Mesnik 
wrote:

> Hi
>
> Currently there were no timeouts wit fastdebug were observed. I don’t know
> exactly how  Xint affects this test but don’t expect significant
> performance degradation comparing with other tests.  Usually timeoutfactor
> is used to increase timeouts of all tests if VM or host are slow. However
> this test launches a lot of VMs with small applications and significantly
> depends on JVM startup performance. I don’t think that other ‘slow’ modes
> affect  startup performance so significantly as Xcomp.
>
> Leonid
>
> On Jul 28, 2017, at 2:17 PM, Martin Buchholz  wrote:
>
> Won't this test fail with any "slow" VM, e.g. fastdebug, -Xint, etc ?
>
>
> On Fri, Jul 28, 2017 at 2:05 PM, Leonid Mesnik 
> wrote:
>
>> Hi
>>
>> Please review following small fix which excludes test
>> tools/launcher/modules/illegalaccess/IllegalAccessTest.java from
>> execution in Xcomp mode. Test launches about 100 VMs and fails when -Xcomp
>> is used.
>>
>> Tested locally with and without adding -Xcomp option.
>>
>> Webrev: http://cr.openjdk.java.net/~lmesnik/8184775/webrev.00/ <
>> http://cr.openjdk.java.net/~lmesnik/8184775/webrev.00/>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8184775 <
>> https://bugs.openjdk.java.net/browse/JDK-8184775>
>> Diff:
>> --- old/test/tools/launcher/modules/illegalaccess/IllegalAccessTest.java
>>   2017-07-27 17:15:37.0 -0700
>> +++ new/test/tools/launcher/modules/illegalaccess/IllegalAccessTest.java
>>   2017-07-27 17:15:37.0 -0700
>> @@ -23,6 +23,7 @@
>>
>>  /**
>>   * @test
>> + * @requires vm.compMode != "Xcomp"
>>   * @modules java.base/jdk.internal.misc
>>   *  java.base/sun.security.x509
>>   *  java.activation
>>
>> Leonid
>
>
>
>

Re: RFR(XS) 8184775: tools/launcher/modules/illegalaccess/IllegalAccessTest.java times out on some platforms…

[Leonid Mesnik]

Hi

Currently there were no timeouts wit fastdebug were observed. I don’t know 
exactly how  Xint affects this test but don’t expect significant performance 
degradation comparing with other tests.  Usually timeoutfactor is used to 
increase timeouts of all tests if VM or host are slow. However this test 
launches a lot of VMs with small applications and significantly depends on JVM 
startup performance. I don’t think that other ‘slow’ modes affect  startup 
performance so significantly as Xcomp.  

Leonid

> On Jul 28, 2017, at 2:17 PM, Martin Buchholz  wrote:
> 
> Won't this test fail with any "slow" VM, e.g. fastdebug, -Xint, etc ?
> 
> 
> On Fri, Jul 28, 2017 at 2:05 PM, Leonid Mesnik  > wrote:
> Hi
> 
> Please review following small fix which excludes test 
> tools/launcher/modules/illegalaccess/IllegalAccessTest.java from execution in 
> Xcomp mode. Test launches about 100 VMs and fails when -Xcomp is used.
> 
> Tested locally with and without adding -Xcomp option.
> 
> Webrev: http://cr.openjdk.java.net/~lmesnik/8184775/webrev.00/ 
>  
>  >
> Bug: https://bugs.openjdk.java.net/browse/JDK-8184775 
>  
>  >
> Diff:
> --- old/test/tools/launcher/modules/illegalaccess/IllegalAccessTest.java  
>   2017-07-27 17:15:37.0 -0700
> +++ new/test/tools/launcher/modules/illegalaccess/IllegalAccessTest.java  
>   2017-07-27 17:15:37.0 -0700
> @@ -23,6 +23,7 @@
> 
>  /**
>   * @test
> + * @requires vm.compMode != "Xcomp"
>   * @modules java.base/jdk.internal.misc
>   *  java.base/sun.security.x509
>   *  java.activation
> 
> Leonid
> 

Re: RFR(XS) 8184775: tools/launcher/modules/illegalaccess/IllegalAccessTest.java times out on some platforms…

[Alan Bateman]

On 28/07/2017 22:17, Martin Buchholz wrote:

Won't this test fail with any "slow" VM, e.g. fastdebug, -Xint, etc ?
When testing with debug builds or -Xint or -Xcomp then default timeout 
is often insufficient so the recommendation has always been to specify 
-timeoutFactor to jtreg.


This test is dominated by startup and can take a long time with a debug 
+ -Xcomp run. I think Leonid found a particularly slow machine too. So I 
think skipping it for -Xcomp runs should be okay as this test is 
unlikely to helping finding compiler/VM bugs.


-Alan

Re: RFR(XS) 8184775: tools/launcher/modules/illegalaccess/IllegalAccessTest.java times out on some platforms…

[Martin Buchholz]

Won't this test fail with any "slow" VM, e.g. fastdebug, -Xint, etc ?


On Fri, Jul 28, 2017 at 2:05 PM, Leonid Mesnik 
wrote:

> Hi
>
> Please review following small fix which excludes test
> tools/launcher/modules/illegalaccess/IllegalAccessTest.java from
> execution in Xcomp mode. Test launches about 100 VMs and fails when -Xcomp
> is used.
>
> Tested locally with and without adding -Xcomp option.
>
> Webrev: http://cr.openjdk.java.net/~lmesnik/8184775/webrev.00/ <
> http://cr.openjdk.java.net/~lmesnik/8184775/webrev.00/>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8184775 <
> https://bugs.openjdk.java.net/browse/JDK-8184775>
> Diff:
> --- old/test/tools/launcher/modules/illegalaccess/IllegalAccessTest.java
>   2017-07-27 17:15:37.0 -0700
> +++ new/test/tools/launcher/modules/illegalaccess/IllegalAccessTest.java
>   2017-07-27 17:15:37.0 -0700
> @@ -23,6 +23,7 @@
>
>  /**
>   * @test
> + * @requires vm.compMode != "Xcomp"
>   * @modules java.base/jdk.internal.misc
>   *  java.base/sun.security.x509
>   *  java.activation
>
> Leonid

RFR(XS) 8184775: tools/launcher/modules/illegalaccess/IllegalAccessTest.java times out on some platforms…

[Leonid Mesnik]

Hi

Please review following small fix which excludes test 
tools/launcher/modules/illegalaccess/IllegalAccessTest.java from execution in 
Xcomp mode. Test launches about 100 VMs and fails when -Xcomp is used.

Tested locally with and without adding -Xcomp option.

Webrev: http://cr.openjdk.java.net/~lmesnik/8184775/webrev.00/ 

Bug: https://bugs.openjdk.java.net/browse/JDK-8184775 

Diff:
--- old/test/tools/launcher/modules/illegalaccess/IllegalAccessTest.java
2017-07-27 17:15:37.0 -0700
+++ new/test/tools/launcher/modules/illegalaccess/IllegalAccessTest.java
2017-07-27 17:15:37.0 -0700
@@ -23,6 +23,7 @@
 
 /**
  * @test
+ * @requires vm.compMode != "Xcomp"
  * @modules java.base/jdk.internal.misc
  *  java.base/sun.security.x509
  *  java.activation

Leonid

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Jonathan Bluett-Duncan]

Hi Ivan,

It looks like the MyComparator code example which you gave in your last
email lost its formatting along the way, so I'm finding it difficult to
read. Would you mind resubmitting it?

Cheers,
Jonathan

On 28 July 2017 at 17:25, Ivan Gerasimov  wrote:

> Hi Peter!
>
> Thank a lot for looking into this!
>
> On 7/28/17 7:32 AM, Peter Levart wrote:
>
>> Hi Ivan,
>>
>> In the light of what Stuart Marks wrote, then what do you think about a
>> parameterized comparator (would be sub-optimal, I know) where one would
>> supply
>> 2 Comparator(s) which would be used to compare Ax and Nx sub-sequences
>> respectively as described below...
>>
>> Yes.  Inspired by what Stuart suggested I made a draft of such a
> comparator (see below).
> It works, but as you've said it's not that efficient (mostly due to
> expensive substrings) and a bit harder to use in a simple case.
> Now I need to think about how to combine two approaches.
>
> For Nx sub-sequences, one would need a comparator comparing the reversed
>> sequence lexicographically,
>>
> I'm not sure I understand why they need to be reversed.
>
>> but for Ax sub-sequences, one could choose from a plethora of
>> case-(in)sensitive comparator(s) and collators already available on the
>> platform.
>>
>> Yes. In the example below I used compareToIgnoreCase to compare alpha
> subsequences.
>
> ---
>
> class MyComparator implements Comparator {Comparator
> alphaCmp;Comparator numCmp;MyComparator(Comparator
> alphaCmp,Comparator numCmp) {this.alphaCmp = alphaCmp;this.numCmp =
> numCmp;}boolean skipLeadingZeroes(String s, int len) {for (int i = 0; i <
> len ; ++i) {if (Character.digit(s.charAt(i), 10) != 0)return false;}return
> true;}@Override public int compare(String o1, String o2)
> {Supplier s1 = new NumberSlicer(o1);Supplier s2 = new
> NumberSlicer(o2);while (true) {// alpha part String ss1 = s1.get();String
> ss2 = s2.get();int cmp = alphaCmp.compare(ss1, ss2);if (cmp != 0) return
> cmp;if (ss1.length() == 0) return 0;// numeric part ss1 = s1.get();ss2 =
> s2.get();int len1 = ss1.length();int len2 = ss2.length();int dlen = len1 -
> len2;if (dlen > 0) {if (!skipLeadingZeroes(ss1, dlen))return 1;ss1 =
> ss1.substring(dlen, len1);} else if (dlen < 0) {if (!skipLeadingZeroes(ss2,
> -dlen))return -1;ss2 = ss2.substring(-dlen, len2);}cmp =
> numCmp.compare(ss1, ss2);if (cmp != 0) return cmp;if (dlen != 0) return
> dlen;}}static class NumberSlicer implements Supplier {private
> String sequence;private boolean expectNumber = false;private int index =
> 0;NumberSlicer(String s) {sequence = s;}@Override public String get()
> {int start = index, end = start, len = sequence.length() - start;for (; len
> > 0; ++end, --len) {if (Character.isDigit(sequence.charAt(end)) ^
> expectNumber)break;}expectNumber = !expectNumber;return
> sequence.substring(start, index = end);}}}Here how it is
> invoked with case-insensitive comparator:Arrays.sort(str,new
> MyComparator(Comparator.comparing(String::toString,String::
> compareToIgnoreCase),Comparator.comparing(String::toString,
> String::compareTo)));
>
> simple test results for case insensitive sorting:java 1java 1 javajava 1
> JAVAJava 2JAVA 5jaVA 6.1java 10java 10 v 13Java 10 v 013Java 10 v
> 13java 10 v 113Java 2017Java 2017Java 20017Java 200017Java 217Java
> 2017Java 2017Java 20017With kind regards, Ivan
>

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[joe darcy]

Hi Ivan,

A few comments.

I don't have a specific suggestion for a different name, but I don't 
think "comparingNumerically" does an adequate job capturing the 
described behavior of the method. Likewise, the summary of the methods' 
behavior in the javadoc should have a more suggestive description of the 
behavior.


Please add comments describing what the "// Fullwidth characters" values 
are in the test.


Interesting test cases would also include substrings for 
Integer.MAX_VALUE, Integer.MAX_VALUE+1, Long.MAX_VALUE, Long.MAX_VALUE + 
1, etc. Test cases of purely numerical and purely alphabetical inputs 
would be warranted too.


While randomizing the array and sorting is a valid test, a more through 
test would do pair-wise comparisons of each array element. While such 
comparisons are quadratic with array length, the arrays here are small.


Cheers,

-Joe


On 7/28/2017 9:03 AM, Ivan Gerasimov wrote:

Hi Roger!


On 7/24/17 7:42 AM, Roger Riggs wrote:

Hi Ivan,

Thanks for bringing this up again.

Some initial comments, not a full review.

Though the enhancement says that no consideration is given to sign 
characters they
may produce puzzling results for strings like "-2000" and "-2001" 
where the strings appear
to be signed numbers but the comparison will be for the "-" prefix 
and the rest unsigned.
Including the word 'unsigned' in the description might help reinforce 
the semantics.


Yes, it's a good point.  I'll add some words to make it certain we 
only recognize unsigned numbers.
Otherwise, it would become confusing when comparing something like 
"2017-07-28" and "2017-07-29".


Also, I don't see test cases for strings like: "abc200123" and 
"abc2123" which share
a prefix part of which is numeric but differ in the number of 
"leading" zeros after the prefix.



Sure.  Good addition to the test.

What about strings that include more than 1 numeric segment: 
abc2003def0001 and abc02003def001

in the leading zero case?

With the first comparator (which treats the numbers with more leading 
zeroes as greater ones), these strings would be sorted as 
"abc2003def0001" < "abc02003def001".

The logic here is as following:
1) skip common prefix "abc",
2) find the numerical parts: "2003" and "02003",
3) after skipping leading zeroes, they are compared to be equal,
4) then, the string with more leading zeroes is said to be greater.

Though the test adds the @key randomness, it would be useful to use 
the test library
mechanisms to report the seed and be able to run the test with a 
seed, if case they fail.

(As might be provided by the test library jdk.test.lib.RandomFactory).


Okay, I'll add this Random to the shuffling to make it reproducible.


Comparator.java:
576: "the common prefix is skipped" is problematic when considering 
leading zeros.

   The common prefix may contain non-zero digits.

Yes, of course.
While scanning the string for the common prefix, we still keep track 
of the numeric part.

Probably "skip" is a wrong word here.

585: it is not clear whether the "different number of leading zeros" 
is applied regardless

   of the common prefix or only starting after the common prefix.

When talking about leading zeroes, then the entire numerical substring 
is meant.

Part of this substring can belong to the common prefix.

550, 586: for many readers, it is easier to read 'for example' than 
"e.g." or "i.e.".



Thanks!  I'll change it accordingly.


562, 598:  editorial: "is to compare" -> "compares"


Thanks!

Comparators.java:

192: @param for param o is missing;  (the param name "o" usually 
refers to an object, not a string).
200:   Can skipLeadingZeros be coded to correctly work if cnt == 0; 
it would be more robust
 SkipLeading zeros works correctly only if pos is given the first 
numeric digit in the subsequence

 so the numStart1 and numStart2 must be first digit in each string.
I don't think that skipLeadingZeros() is very specific that it always 
called for longer string, trying to reduce its size via skipping 
leading zeroes.
It wouldn't make sense to call it with cnt == 0, and so we can avoid 
one initial comparing and save a couple of nanoseconds :)
I added this prerequisite to the javadoc of the method, so hopefully 
it shouldn't cause much confusion.



compare():

Line 223: if strings typically have non-numeric prefixes, it might 
perform slightly better
to detect the end of the common prefix and then scan back to find the 
beginning of the numeric part.

(Avoiding checking every char for isDigit).

On the other hand, we're saving on not calling String.charAt() for the 
second time :)


Line 224: If assigned for every digit, it will hold the last equal 
digit in the common prefix, not the first digit.

It will only be assigned when a non-digit is met.
And since the index was just incremented @ Line 222, numStart1 will be 
set to the index of the first digit inside the common prefix.


For example, if the common prefix is abc123, then the numStart1 will 
be 

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Ivan Gerasimov]

Hi Peter!

Thank a lot for looking into this!

On 7/28/17 7:32 AM, Peter Levart wrote:

Hi Ivan,

In the light of what Stuart Marks wrote, then what do you think about 
a parameterized comparator (would be sub-optimal, I know) where one 
would supply
2 Comparator(s) which would be used to compare Ax and Nx sub-sequences 
respectively as described below...


Yes.  Inspired by what Stuart suggested I made a draft of such a 
comparator (see below).
It works, but as you've said it's not that efficient (mostly due to 
expensive substrings) and a bit harder to use in a simple case.

Now I need to think about how to combine two approaches.

For Nx sub-sequences, one would need a comparator comparing the 
reversed sequence lexicographically,

I'm not sure I understand why they need to be reversed.
but for Ax sub-sequences, one could choose from a plethora of 
case-(in)sensitive comparator(s) and collators already available on 
the platform.


Yes. In the example below I used compareToIgnoreCase to compare alpha 
subsequences.


---

class MyComparator implements Comparator {Comparator 
alphaCmp;Comparator numCmp;MyComparator(Comparator 
alphaCmp,Comparator numCmp) {this.alphaCmp = 
alphaCmp;this.numCmp = numCmp;}boolean skipLeadingZeroes(String s, int 
len) {for (int i = 0; i < len ; ++i) {if (Character.digit(s.charAt(i), 
10) != 0)return false;}return true;}@Override public int compare(String 
o1, String o2) {Supplier s1 = new 
NumberSlicer(o1);Supplier s2 = new NumberSlicer(o2);while (true) 
{// alpha part String ss1 = s1.get();String ss2 = s2.get();int cmp = 
alphaCmp.compare(ss1, ss2);if (cmp != 0) return cmp;if (ss1.length() == 
0) return 0;// numeric part ss1 = s1.get();ss2 = s2.get();int len1 = 
ss1.length();int len2 = ss2.length();int dlen = len1 - len2;if (dlen > 
0) {if (!skipLeadingZeroes(ss1, dlen))return 1;ss1 = ss1.substring(dlen, 
len1);} else if (dlen < 0) {if (!skipLeadingZeroes(ss2, -dlen))return 
-1;ss2 = ss2.substring(-dlen, len2);}cmp = numCmp.compare(ss1, ss2);if 
(cmp != 0) return cmp;if (dlen != 0) return dlen;}}static class 
NumberSlicer implements Supplier {private String 
sequence;private boolean expectNumber = false;private int index = 
0;NumberSlicer(String s) {sequence = s;}@Override public String get() 
{int start = index, end = start, len = sequence.length() - start;for (; 
len > 0; ++end, --len) {if (Character.isDigit(sequence.charAt(end)) ^ 
expectNumber)break;}expectNumber = !expectNumber;return 
sequence.substring(start, index = end);}}}Here how it is 
invoked with case-insensitive comparator:Arrays.sort(str,new 
MyComparator(Comparator.comparing(String::toString,String::compareToIgnoreCase),Comparator.comparing(String::toString,String::compareTo)));


simple test results for case insensitive sorting:java 1java 1 javajava 1 
JAVAJava 2JAVA 5jaVA 6.1java 10java 10 v 13Java 10 v 013Java 10 v 
13java 10 v 113Java 2017Java 2017Java 20017Java 200017Java 
217Java 2017Java 2017Java 20017With kind regards, Ivan

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Ivan Gerasimov]

Hi Roger!


On 7/24/17 7:42 AM, Roger Riggs wrote:

Hi Ivan,

Thanks for bringing this up again.

Some initial comments, not a full review.

Though the enhancement says that no consideration is given to sign 
characters they
may produce puzzling results for strings like "-2000" and "-2001" 
where the strings appear
to be signed numbers but the comparison will be for the "-" prefix and 
the rest unsigned.
Including the word 'unsigned' in the description might help reinforce 
the semantics.


Yes, it's a good point.  I'll add some words to make it certain we only 
recognize unsigned numbers.
Otherwise, it would become confusing when comparing something like 
"2017-07-28" and "2017-07-29".


Also, I don't see test cases for strings like: "abc200123" and 
"abc2123" which share
a prefix part of which is numeric but differ in the number of 
"leading" zeros after the prefix.



Sure.  Good addition to the test.

What about strings that include more than 1 numeric segment: 
abc2003def0001 and abc02003def001

in the leading zero case?

With the first comparator (which treats the numbers with more leading 
zeroes as greater ones), these strings would be sorted as 
"abc2003def0001" < "abc02003def001".

The logic here is as following:
1) skip common prefix "abc",
2) find the numerical parts: "2003" and "02003",
3) after skipping leading zeroes, they are compared to be equal,
4) then, the string with more leading zeroes is said to be greater.

Though the test adds the @key randomness, it would be useful to use 
the test library
mechanisms to report the seed and be able to run the test with a seed, 
if case they fail.

(As might be provided by the test library jdk.test.lib.RandomFactory).


Okay, I'll add this Random to the shuffling to make it reproducible.


Comparator.java:
576: "the common prefix is skipped" is problematic when considering 
leading zeros.

   The common prefix may contain non-zero digits.

Yes, of course.
While scanning the string for the common prefix, we still keep track of 
the numeric part.

Probably "skip" is a wrong word here.

585: it is not clear whether the "different number of leading zeros" 
is applied regardless

   of the common prefix or only starting after the common prefix.

When talking about leading zeroes, then the entire numerical substring 
is meant.

Part of this substring can belong to the common prefix.

550, 586: for many readers, it is easier to read 'for example' than 
"e.g." or "i.e.".



Thanks!  I'll change it accordingly.


562, 598:  editorial: "is to compare" -> "compares"


Thanks!

Comparators.java:

192: @param for param o is missing;  (the param name "o" usually 
refers to an object, not a string).
200:   Can skipLeadingZeros be coded to correctly work if cnt == 0; it 
would be more robust
 SkipLeading zeros works correctly only if pos is given the first 
numeric digit in the subsequence

 so the numStart1 and numStart2 must be first digit in each string.
I don't think that skipLeadingZeros() is very specific that it always 
called for longer string, trying to reduce its size via skipping leading 
zeroes.
It wouldn't make sense to call it with cnt == 0, and so we can avoid one 
initial comparing and save a couple of nanoseconds :)
I added this prerequisite to the javadoc of the method, so hopefully it 
shouldn't cause much confusion.



compare():

Line 223: if strings typically have non-numeric prefixes, it might 
perform slightly better
to detect the end of the common prefix and then scan back to find the 
beginning of the numeric part.

(Avoiding checking every char for isDigit).

On the other hand, we're saving on not calling String.charAt() for the 
second time :)


Line 224: If assigned for every digit, it will hold the last equal 
digit in the common prefix, not the first digit.

It will only be assigned when a non-digit is met.
And since the index was just incremented @ Line 222, numStart1 will be 
set to the index of the first digit inside the common prefix.


For example, if the common prefix is abc123, then the numStart1 will be 
updated to 3 when looking at the char 'c'.  Last three cycles of the 
loop it won't be updated, since all the trailing chars are digits.




239, 240:  chars at o1(index) and o2(index) are already known to be 
digits, can it be avoided checking them twice

by starting at index+1?

Not quite necessarily.  We can be here due to numLen1 > 0, and *only 
one* or *none* of the string remainders start with digits.

In the later case we'll end up @ line 279.

Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8134512/03/webrev/

Given the further discussion this iteration is probably not the final, 
but it's better to have a checkpoint :)


With kind regards,
Ivan


$.02, Roger


On 7/19/2017 4:41 AM, Ivan Gerasimov wrote:

Hello!

It is a proposal to provide a String comparator, which will pay 
attention to the numbers embedded into the strings (should they 
present).


This proposal was initially discussed 

Re: RFR: JDK-8183579: refactor and cleanup launcher help messages

[Kumar Srinivasan]

Mandy,

Thanks for the reviewplease see in-lined comments,


On Jul 20, 2017, at 11:53 AM, Kumar Srinivasan  
wrote:

Hi,

Please review refactoring and clean up of the java launcher's help/usage
messages.

The highlights of the changes are as follows:

1. Helper.java: is renamed from LauncherHelper.java, simpler name,
containing mostly methods to help with class loading, module assistance etc.


There are tests depending on `sun.launcher.LauncherHelper` class name.
I actually like LauncherHelper better than Helper to make it clear
what it is just by its simple name.

The rationale was to simplify the fullname from
sun.launcher.LauncherHelper -> sun.launcher.Helper

wrt. the test Ugh, Ok renamed Helper.java back to LauncherHelper.java


Webrev:
http://cr.openjdk.java.net/~ksrini/8183579/webrev.00/

launcher.properties
   40 # Translators please note do not translate the options themselves

Option names are no longer in the resource bundle.
It would be helpful to add the comment describing the key name
   java.launcher.opt.$OPTION_NAME.param
   java.launcher.opt.$OPTION_NAME.desc


Added the notes/comments describing what needs to be done wrt.
adding any new keys.


A newline between each option may help readability.
Since the whitespace in description is irrelevant, maybe keep the indentation
to 4 space?


Done


Does the help message show the options in the order listed here?


In the declaration order.


I would hope it uses the order of the Option enums.  If so, we could
list them in alphabetical order in launcher.properties.


The keys are alpha sorted now.


   80 java.launcher.opt.x.desc = print help on extra options to the error stream

Should $OPTION_NAME match the option name (rather than toLowerCase)?


Ok used the Enum name directly with no translations.


OptionsHelper.java
  311 Arrays.asList(Option.values()).stream()

Alternative: Stream(Option.values())


Done.


This class includes the entry point for -XshowSettings but not other options 
such as —-list-modules.  It may cause confusion to which class a new launcher 
option is added.  It may be okay.   Maybe just move the code for printing the 
help messages in Option class and consider the refactoring for -XshowSetting 
and other options in the future.


As you suggested, moved only the two usage helper methods to the new 
UsageHelper.


This simplifies the changes.

New webrev at:
http://cr.openjdk.java.net/~ksrini/8183579/webrev.01/

Thanks

Kumar


Mandy

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Peter Levart]

Hi Ivan,

In the light of what Stuart Marks wrote, then what do you think about a 
parameterized comparator (would be sub-optimal, I know) where one would 
supply
2 Comparator(s) which would be used to compare Ax and Nx sub-sequences 
respectively as described below...


For Nx sub-sequences, one would need a comparator comparing the reversed 
sequence lexicographically, but for Ax sub-sequences, one could choose 
from a plethora of case-(in)sensitive comparator(s) and collators 
already available on the platform.


Regards, peter

On 07/28/2017 04:12 PM, Peter Levart wrote:

Hi Ivan,

Would I be wrong if I described the logic of these two comparators as 
the following:


The comparator compares two character sequences as though each of them 
would be 1st transformed into a tuple of the form:


(A0, N0, A1, N1, ..., An-1, Nn-1, An)

where:

A0 and An are (possibly empty) sub-sequences consisting of 
non-decimal-digit characters
A1 ... An-1 are non-empty sub-sequences consisting of 
non-decimal-digit characters
N0 ... Nn-1 are non-empty sub-sequences consisting of decimal-digit 
characters


...such that all sub-sequences concatenated together in order as they 
appear in the tuple yield the original character sequence.


Any characher sequence can be uniquely transformed into such tuple. 
For example:


"" -> (A0); where A0 == ""
"ab10" -> (A0, N0, A1); where A0 == "ab", N0 == "10", A1 = ""
"1" -> (A0, N0, A1); where A0 == "", N0 == "1", A1 = ""
...

After transformation, the tuples are compared by their elements (from 
left to right) so that corresponding Ax elements are compared 
lexicographically and Nx elements are compared as decimal integers 
(with two variations considering leading zeroes).


If I am right than perhaps such description would be easier to 
understand.


What do you think?


Regards, Peter

On 07/19/2017 10:41 AM, Ivan Gerasimov wrote:

Hello!

It is a proposal to provide a String comparator, which will pay 
attention to the numbers embedded into the strings (should they 
present).


This proposal was initially discussed back in 2014 and seemed to 
bring some interest from the community:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-December/030343.html 



In the latest webrev two methods are added to the public API:
j.u.Comparator.comparingNumerically() and
j.u.Comparator.comparingNumericallyLeadingZerosAhead().

The regression test is extended to exercise this new comparator.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8134512
WEBREV: http://cr.openjdk.java.net/~igerasim/8134512/01/webrev/

Comments, suggestions are very welcome!

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Peter Levart]

Hi Ivan,

Would I be wrong if I described the logic of these two comparators as 
the following:


The comparator compares two character sequences as though each of them 
would be 1st transformed into a tuple of the form:


(A0, N0, A1, N1, ..., An-1, Nn-1, An)

where:

A0 and An are (possibly empty) sub-sequences consisting of 
non-decimal-digit characters
A1 ... An-1 are non-empty sub-sequences consisting of non-decimal-digit 
characters
N0 ... Nn-1 are non-empty sub-sequences consisting of decimal-digit 
characters


...such that all sub-sequences concatenated together in order as they 
appear in the tuple yield the original character sequence.


Any characher sequence can be uniquely transformed into such tuple. For 
example:


"" -> (A0); where A0 == ""
"ab10" -> (A0, N0, A1); where A0 == "ab", N0 == "10", A1 = ""
"1" -> (A0, N0, A1); where A0 == "", N0 == "1", A1 = ""
...

After transformation, the tuples are compared by their elements (from 
left to right) so that corresponding Ax elements are compared 
lexicographically and Nx elements are compared as decimal integers (with 
two variations considering leading zeroes).


If I am right than perhaps such description would be easier to understand.

What do you think?


Regards, Peter

On 07/19/2017 10:41 AM, Ivan Gerasimov wrote:

Hello!

It is a proposal to provide a String comparator, which will pay 
attention to the numbers embedded into the strings (should they present).


This proposal was initially discussed back in 2014 and seemed to bring 
some interest from the community:
http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-December/030343.html 



In the latest webrev two methods are added to the public API:
j.u.Comparator.comparingNumerically() and
j.u.Comparator.comparingNumericallyLeadingZerosAhead().

The regression test is extended to exercise this new comparator.

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8134512
WEBREV: http://cr.openjdk.java.net/~igerasim/8134512/01/webrev/

Comments, suggestions are very welcome!

Re: Review Request: JDK-8161121: VM::isSystemDomainLoader should consider platform class loader

[Alan Bateman]

On 28/07/2017 06:00, Mandy Chung wrote:

With deprivileging, several modules of the runtime are mow defined to
the platform class loader.  VM::isSystemDomainLoader is extended to
detect if the given class loader is boot loader or platform loader.

Webrev:
   http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8161121/webrev.00/index.html


Looks okay to me.

-Alan

Re: [10] RFR 8134512 : provide Alpha-Numeric (logical) Comparator

[Ivan Gerasimov]

Hi Jonathan!

On 7/24/17 3:42 AM, Jonathan Bluett-Duncan wrote:

You're welcome Ivan!

I'm just proofreading the new webrev, and a few more things have 
occurred to me:


1. What happens if the comparators are given the elements {"1abc", 
"2abc", "10abc"} to compare? Would they sort the elements as {"1abc", 
"2abc", "10abc"} or as {"1abc", "10abc", "2abc"}?

What about the array {"x1yz", "x2yz", "x10yz"}?
I wonder if these two cases should be added to the test too and given 
as additional examples in the javadocs.


These arrays would be sorted in the way you expect: i.e. {"1abc", 
"2abc", "10abc"} and {"x1yz", "x2yz", "x10yz"}, respectively.
I agree it is worthwhile to choose a good descriptive example for the 
javadoc, so it would make it clear for a reader what to expect from the 
routine.

Probably, something similar to what they have in MSDN:
https://msdn.microsoft.com/en-us/library/windows/desktop/bb759947(v=vs.85).aspx

2. The example arrays which you kindly added to the comparators' 
javadoc have no whitespace at the start but one space between the last 
element and the }, so I think there either should be no space at the 
end or an extra space at the start.



Okay, I'll make it consistent.

3. Very sorry, error on my part: on the javadoc lines which now say 
"then the corresponding numerical values are compared; otherwise", I 
suggested to add a "then" at the start; re-reading it though, I 
personally think it reads better without, but I would understand and 
not press it further if you disagree and think that the "then" is a 
useful addition.



Fixed, thanks!

I'll post the updated webrev later, once incorporate other suggestions.

With kind regards,
Ivan




Best regards,
Jonathan


On 24 Jul 2017 06:21, "Ivan Gerasimov" > wrote:


Thank you Jonathan for looking into that!

I agree with all your suggestions.

Here's the updated webrev with suggested modifications:
WEBREV: http://cr.openjdk.java.net/~igerasim/8134512/02/webrev/


With kind regards,
Ivan


On 7/23/17 3:56 AM, Jonathan Bluett-Duncan wrote:

Meant to reply to core-libs-dev as well; doing so now.

Jonathan

On 23 July 2017 at 11:50, Jonathan Bluett-Duncan
> wrote:

Hi Ivan,

I'm not a reviewer per se, but I thought I'd chime in.

There's a few things with Comparator.java which I think could
be improved:

 1. `comparingNumericallyLeadingZerosAhead()` is a confusing
name for me, as the "ahead" has no meaning in my mind;
IMO the name `comparingNumericallyLeadingZerosFirst()`
better implies that "0001" < "001" < "01".
 2. It wasn't clear to me from the javadoc that the
comparators compare strings like "abc9" and "abc10" as
"abc9" < "abc10", so I think they should include more
examples.
 3. There's a typo in the javadocs for both methods:
"represets" --> "represents".
 4. Where the javadocs say "follows the procedure", I think
it'd make more grammatical sense if they said "does the
following".
 5. The lines which say "at the boundary of a subsequence,
consisting of decimal digits, the" would flow better if
they said "at the boundary of a subsequence *consisting
solely of decimal digits*, then the". Note the removal of
the comma between "subsequence" and "consisting".

Hope this helps!

Jonathan

On 23 July 2017 at 05:36, Ivan Gerasimov
> wrote:

Hello!

This is a gentle reminder.

The proposed comparator implementation would be
particularly useful when one will need to compare version
strings.
Some popular file managers also use similar comparing
algorithm, as the results often look more natural to the
human eyes (e.g. File Explorer on Windows, Files on Ubuntu).

Now, as Java 10 is been worked on, to sort the list of
Java names correctly, this kind of comparator is needed:

Look: a list { ... "Java 8", "Java 9", "Java 10" }
definitely looks nicer than { "Java 1", "Java 10", "Java
2", ... }  :-)

Would you please help review the proposal?

With kind regards,
Ivan



On 7/19/17 1:41 AM, Ivan Gerasimov wrote:

Hello!

It is a proposal to provide a String comparator,
which will pay attention to the numbers embedded into
the strings (should they present).

This proposal was initially discussed back in 2014