Re: RFR [14/java.xml] 8068376: Validator fails valid XML files due to String == in XSD validator code

[Joe Wang]

Thanks Lance!

-Joe

On 7/25/19 4:04 PM, Lance Andersen wrote:

Hi Joe

The change looks reasonable :-)

On Jul 25, 2019, at 6:28 PM, Joe Wang > wrote:


Please review a quite fix in a validation source where a String 
comparison was made as references.


JBS: https://bugs.openjdk.java.net/browse/JDK-8068376
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8068376/webrev/

Thanks,
Joe




Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com 

Re: RFR [14/java.xml] 8068376: Validator fails valid XML files due to String == in XSD validator code

[Lance Andersen]

Hi Joe

The change looks reasonable :-)

> On Jul 25, 2019, at 6:28 PM, Joe Wang  wrote:
> 
> Please review a quite fix in a validation source where a String comparison 
> was made as references.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8068376
> webrev: http://cr.openjdk.java.net/~joehw/jdk14/8068376/webrev/
> 
> Thanks,
> Joe

 
  

 Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com 

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

> On Jul 25, 2019, at 3:42 PM, Brian Burkhalter  
> wrote:
> 
> the deletion order is (I think) file1, file2, file3 which is the reverse of 
> the order of initial registration.

I intended *not* the reverse order of initial registration.

Brian

Re: 8193072: File.delete() should remove its path from DeleteOnExitHook.files

[Brian Burkhalter]

> On Jul 11, 2019, at 12:52 AM, Peter Levart  wrote:
> 
> On 7/11/19 9:47 AM, Peter Levart wrote:
>> 
>> http://cr.openjdk.java.net/~plevart/jdk-dev/8193072_File.undoDeleteOnExit/webrev.02/
>>  
>> 
>>  
>> 
> 
> Another thing to consider (done in above webrev.02) is what to do with 
> unbalanced cancelDeleteOnExit(). I think it is better to throw exception than 
> to silently ignore it. This way unintentional bugs can be uncovered which 
> would otherwise just cause erratic behavior.

The above patch looks pretty good but unless I am not comprehending the code I 
think there may still be behavioral incompatibilities which might not be 
acceptable. For example, for the existing code base with the following sequence 
of operations

File file1, file2, file3;

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();

the deletion order is file3, file2, file1. For the proposed patch with the 
following sequence of operations

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();
file1.cancelDeleteOnExit(); // file1 is removed from the map
file2.cancelDeleteOnExit(); // file2 is removed from the map
file2.createNewFIle();
file2.deleteOnExit(); // file2 is added back into the map
file1.createNewFIle();
file1.deleteOnExit(); // file1 is added back into the map

the deletion order is (I think) file1, file2, file3 which is the reverse of the 
order of initial registration. Of course it is conceivable to change the 
specification but that seems dangerous somehow. Also for the patch above for 
this sequence of operations

file1.deleteOnExit();
file2.deleteOnExit();
file3.deleteOnExit();
file1.cancelDeleteOnExit(); // file1 is removed from the map
file2.cancelDeleteOnExit(); // file2 is removed from the map
file2.createNewFIle();
file1.createNewFIle();

then file3 is deleted on exit but file1 and file2 are not which differs from 
current behavior.

As Roger wrote

> On Jul 9, 2019, at 7:34 AM, Roger Riggs  wrote:
> 
> The filename is the key and there is no way to determine if it is the 
> original file or a replacement. deleteOnExit Wins!

the filename is the key and if we toss the key then we lose the order of 
registration. Given this I am not sure any more that it is possible to fix this 
issue without introducing an incompatible behavioral change.

Thanks,

Brian

RFR [14/java.xml] 8068376: Validator fails valid XML files due to String == in XSD validator code

[Joe Wang]

Please review a quite fix in a validation source where a String 
comparison was made as references.


JBS: https://bugs.openjdk.java.net/browse/JDK-8068376
webrev: http://cr.openjdk.java.net/~joehw/jdk14/8068376/webrev/

Thanks,
Joe

Re: [14] RFR: 8212970: TZ database in "vanguard" format support

[Joe Wang]

Looks all good Naoto :-)

-Joe

On 7/25/19 2:35 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Yes, I only removed not in-use files, i.e., 2 & 4. I sent the previous 
email just after confirming that all tests (open/closed) passed on a 
platform :-)


Naoto

On 7/25/19 2:24 PM, Joe Wang wrote:

Hi Naoto,

The legacy trap :-)

Relevant files:
1. make/data/tzdata/jdk11_backward
2. test/jdk/sun/util/calendar/zi/tzdata/jdk11_backward
3. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_backward
4. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_full_backward

I see you reverted changes to (1) plus removing (2) and (4). (3) and 
(4) differs slightly, but contains the changes previously made in 
(1). Probably not something to worry about since only (3) is 
referenced, and (4) not. Just wonder whether you've checked on this 
one. I assume your build and test passed without any issues.


Best,
Joe

On 7/25/19 1:04 PM, naoto.s...@oracle.com wrote:

Hi Roger,

On 7/25/19 7:47 AM, Roger Riggs wrote:

Hi Naoto,

TestZoneInfo310.java:
With the composition of the tzdir path up and over to the make 
directory for the tzdir

it might be useful to do an explicit check that the directory exists.
That way if the directory structure on the build side changes,
there will be a test failure makine it obvious that the dependency 
has changed.


If the input tz data files, either in "test" tree or "make" tree, 
cannot be located, the test will fail which effectively reports if 
there is a repo structure change. So I believe it is ok as it is.


Aside from it, the latest changes to eliminate the duplicates caused 
that regression test fail. The reason was that there were actually 
two "jdk11_backward" data files each in "tzdata" and "tzdata_jdk" 
test directories, and the contents differ! I am not sure the reason 
why there are two files this way (seems to be so for years), so I 
reverted that exact file as before. Here is the webrev reflected that:


http://cr.openjdk.java.net/~naoto/8212970/webrev.12/

Naoto



Looks fine.

Thanks, Roger



On 7/24/19 6:24 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for the review.

On 7/24/19 2:57 PM, Joe Wang wrote:

Hi Naoto,

The method findNegativeSavings method in 
TzdbZoneRulesProvider.java states that it "Find the minimum 
negative savings". While the result is correct since the rules 
all have the same value for SAVE, I wonder if that's ideal 
conceptually. Given a start LDT, shouldn't it be looking for the 
SAVE in the exact (narrower) date range (e.g. 1981 - 1989 vs 1981 
- max)?.


I believe it is working as such. The end year is retrieved within 
the method (line 879) and only the minimum negative saving values 
within the window is filtered.




NegativeDSTTest verifies the tzdata, that is the adjusted data 
after import, is that correct? I wonder a comment and a bit of 
details in the test summary would be helpful since there is no 
negative data in the test itself.


Good point. It is confusing. I supplied summary text in the test 
(also the similar line in TestZoneRules.java)


Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8212970/webrev.11/

Naoto


Best,
Joe

On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following enhancement:

https://bugs.openjdk.java.net/browse/JDK-8212970

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8212970/webrev.09/

This change aims to support the "vanguard" IANA time zone data 
format, which uses the negative savings and transition time 
beyond a day period. The change basically translates those 
negative savings and transition times, such as 25:00, into the 
ones that the current JDK recognizes, then produces the data 
file "tzdb.dat" at the build time. At the run time, the data 
file is read and interpreted as before. This way the produced 
tzdb.dat is compatible with the prior JDK releases so that the 
TZ Updater can also distribute it as a time zone update.


I have also refactored redundant copy of ZoneRules file in the 
build directory, by dynamically importing the file under src. 
Thus some build related files are modified. I am hoping folks on 
the build-dev can review those changes.


Naoto

Re: RFR: JDK-8227312: Remove pkg bundle from DMG image.

[Alexander Matveev]

Looks fine.

On 7/25/2019 8:24 AM, Kevin Rushforth wrote:

Looks fine.

-- Kevin

On 7/25/2019 6:24 AM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


[1] https://bugs.openjdk.java.net/browse/JDK-8227312

[2] http://cr.openjdk.java.net/~herrick/8227312/webrev.01/

/Andy

Re: [14] RFR: 8212970: TZ database in "vanguard" format support

[naoto . sato]

Hi Joe,

Yes, I only removed not in-use files, i.e., 2 & 4. I sent the previous 
email just after confirming that all tests (open/closed) passed on a 
platform :-)


Naoto

On 7/25/19 2:24 PM, Joe Wang wrote:

Hi Naoto,

The legacy trap :-)

Relevant files:
1. make/data/tzdata/jdk11_backward
2. test/jdk/sun/util/calendar/zi/tzdata/jdk11_backward
3. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_backward
4. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_full_backward

I see you reverted changes to (1) plus removing (2) and (4). (3) and (4) 
differs slightly, but contains the changes previously made in (1). 
Probably not something to worry about since only (3) is referenced, and 
(4) not. Just wonder whether you've checked on this one. I assume your 
build and test passed without any issues.


Best,
Joe

On 7/25/19 1:04 PM, naoto.s...@oracle.com wrote:

Hi Roger,

On 7/25/19 7:47 AM, Roger Riggs wrote:

Hi Naoto,

TestZoneInfo310.java:
With the composition of the tzdir path up and over to the make 
directory for the tzdir

it might be useful to do an explicit check that the directory exists.
That way if the directory structure on the build side changes,
there will be a test failure makine it obvious that the dependency 
has changed.


If the input tz data files, either in "test" tree or "make" tree, 
cannot be located, the test will fail which effectively reports if 
there is a repo structure change. So I believe it is ok as it is.


Aside from it, the latest changes to eliminate the duplicates caused 
that regression test fail. The reason was that there were actually two 
"jdk11_backward" data files each in "tzdata" and "tzdata_jdk" test 
directories, and the contents differ! I am not sure the reason why 
there are two files this way (seems to be so for years), so I reverted 
that exact file as before. Here is the webrev reflected that:


http://cr.openjdk.java.net/~naoto/8212970/webrev.12/

Naoto



Looks fine.

Thanks, Roger



On 7/24/19 6:24 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for the review.

On 7/24/19 2:57 PM, Joe Wang wrote:

Hi Naoto,

The method findNegativeSavings method in TzdbZoneRulesProvider.java 
states that it "Find the minimum negative savings". While the 
result is correct since the rules all have the same value for SAVE, 
I wonder if that's ideal conceptually. Given a start LDT, shouldn't 
it be looking for the SAVE in the exact (narrower) date range (e.g. 
1981 - 1989 vs 1981 - max)?.


I believe it is working as such. The end year is retrieved within 
the method (line 879) and only the minimum negative saving values 
within the window is filtered.




NegativeDSTTest verifies the tzdata, that is the adjusted data 
after import, is that correct? I wonder a comment and a bit of 
details in the test summary would be helpful since there is no 
negative data in the test itself.


Good point. It is confusing. I supplied summary text in the test 
(also the similar line in TestZoneRules.java)


Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8212970/webrev.11/

Naoto


Best,
Joe

On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following enhancement:

https://bugs.openjdk.java.net/browse/JDK-8212970

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8212970/webrev.09/

This change aims to support the "vanguard" IANA time zone data 
format, which uses the negative savings and transition time beyond 
a day period. The change basically translates those negative 
savings and transition times, such as 25:00, into the ones that 
the current JDK recognizes, then produces the data file "tzdb.dat" 
at the build time. At the run time, the data file is read and 
interpreted as before. This way the produced tzdb.dat is 
compatible with the prior JDK releases so that the TZ Updater can 
also distribute it as a time zone update.


I have also refactored redundant copy of ZoneRules file in the 
build directory, by dynamically importing the file under src. Thus 
some build related files are modified. I am hoping folks on the 
build-dev can review those changes.


Naoto

Re: RFR: JDK-8227684 : jpackage must handle win32 mangled names in jli.dll

[Alexander Matveev]

Do you know if all 32-bit builds will have "_JLI_Launch@56"?
Also, I think @56 is ordinal which can be used to load function by this 
number instead of the name, so I do not think you need to specify it.


My suggestion is to try "JLI_Launch" first and then "_JLI_Launch" and 
then "_JLI_Launch@56", if we doing 32-bit build.


Thanks,
Alexander

On 7/25/2019 8:22 AM, Kevin Rushforth wrote:

It looks fine.

-- Kevin


On 7/25/2019 6:20 AM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).



[1] https://bugs.openjdk.java.net/browse/JDK-8227684

[2] http://cr.openjdk.java.net/~herrick/8227684/webrev.01/

Submitted by: Robert Lichtenberger

/Andy

Re: [14] RFR: 8212970: TZ database in "vanguard" format support

[Joe Wang]

Hi Naoto,

The legacy trap :-)

Relevant files:
1. make/data/tzdata/jdk11_backward
2. test/jdk/sun/util/calendar/zi/tzdata/jdk11_backward
3. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_backward
4. test/jdk/sun/util/calendar/zi/tzdata_jdk/jdk11_full_backward

I see you reverted changes to (1) plus removing (2) and (4). (3) and (4) 
differs slightly, but contains the changes previously made in (1). 
Probably not something to worry about since only (3) is referenced, and 
(4) not. Just wonder whether you've checked on this one. I assume your 
build and test passed without any issues.


Best,
Joe

On 7/25/19 1:04 PM, naoto.s...@oracle.com wrote:

Hi Roger,

On 7/25/19 7:47 AM, Roger Riggs wrote:

Hi Naoto,

TestZoneInfo310.java:
With the composition of the tzdir path up and over to the make 
directory for the tzdir

it might be useful to do an explicit check that the directory exists.
That way if the directory structure on the build side changes,
there will be a test failure makine it obvious that the dependency 
has changed.


If the input tz data files, either in "test" tree or "make" tree, 
cannot be located, the test will fail which effectively reports if 
there is a repo structure change. So I believe it is ok as it is.


Aside from it, the latest changes to eliminate the duplicates caused 
that regression test fail. The reason was that there were actually two 
"jdk11_backward" data files each in "tzdata" and "tzdata_jdk" test 
directories, and the contents differ! I am not sure the reason why 
there are two files this way (seems to be so for years), so I reverted 
that exact file as before. Here is the webrev reflected that:


http://cr.openjdk.java.net/~naoto/8212970/webrev.12/

Naoto



Looks fine.

Thanks, Roger



On 7/24/19 6:24 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for the review.

On 7/24/19 2:57 PM, Joe Wang wrote:

Hi Naoto,

The method findNegativeSavings method in TzdbZoneRulesProvider.java 
states that it "Find the minimum negative savings". While the 
result is correct since the rules all have the same value for SAVE, 
I wonder if that's ideal conceptually. Given a start LDT, shouldn't 
it be looking for the SAVE in the exact (narrower) date range (e.g. 
1981 - 1989 vs 1981 - max)?.


I believe it is working as such. The end year is retrieved within 
the method (line 879) and only the minimum negative saving values 
within the window is filtered.




NegativeDSTTest verifies the tzdata, that is the adjusted data 
after import, is that correct? I wonder a comment and a bit of 
details in the test summary would be helpful since there is no 
negative data in the test itself.


Good point. It is confusing. I supplied summary text in the test 
(also the similar line in TestZoneRules.java)


Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8212970/webrev.11/

Naoto


Best,
Joe

On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following enhancement:

https://bugs.openjdk.java.net/browse/JDK-8212970

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8212970/webrev.09/

This change aims to support the "vanguard" IANA time zone data 
format, which uses the negative savings and transition time beyond 
a day period. The change basically translates those negative 
savings and transition times, such as 25:00, into the ones that 
the current JDK recognizes, then produces the data file "tzdb.dat" 
at the build time. At the run time, the data file is read and 
interpreted as before. This way the produced tzdb.dat is 
compatible with the prior JDK releases so that the TZ Updater can 
also distribute it as a time zone update.


I have also refactored redundant copy of ZoneRules file in the 
build directory, by dynamically importing the file under src. Thus 
some build related files are modified. I am hoping folks on the 
build-dev can review those changes.


Naoto

Re: RFR: 8228623 : Update copyright year to 2019 for several java properties file

[naoto . sato]

Hi Leo,

Looks good to me. I rather prefer noreg-l10n other than noreg-doc for 
the Jira issue label.


Naoto

On 7/25/19 9:27 AM, li.ji...@oracle.com wrote:

Hi,

Please review this trivial change to correct the copyright year in 
several java properties file. I updated the copyright year in English, 
Simplified Chinese and Japanese resource files.


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

Webrev:
http://cr.openjdk.java.net/~ljiang/8228623/webrev/

Thanks,
Leo

Re: RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

[Alexander Matveev]

Hi Dmitry,

Looks good.

Thanks,
Alexander

On 7/25/2019 5:38 AM, Dmitry Chuyko wrote:
I realized we also need better detection of arch for deb control file. 
It can be done by using 'dpkg --print-architecture' on host. 
LinuxDebBundler was corrected.


New webrev: http://cr.openjdk.java.net/~dchuyko/8222778/webrev.04/

Patch was re-tested together with webrev.02 from JDK-8228402.

-Dmitry

On 7/25/19 12:56 AM, Alexander Matveev wrote:

Looks good.

On 7/24/2019 7:30 AM, Dmitry Chuyko wrote:
Webrev without chdir() and write() changes: 
http://cr.openjdk.java.net/~dchuyko/8222778/webrev.03/


When it is applied together with webrev.01 [1] from JDK-8228402, 
sandbox is compiled and passes same jpackage tests on x86, aarch64 
and other platforms.


-Dmitry

[1] http://cr.openjdk.java.net/~herrick/8228402/

On 7/19/19 11:31 AM, Andrew Haley wrote:

On 7/19/19 12:36 AM, Dmitry Chuyko wrote:

On 7/18/19 11:57 AM, Andrew Haley wrote:

Hi,

On 7/17/19 10:19 PM, Dmitry Chuyko wrote:

On 7/17/19 11:52 AM, Andrew Haley wrote:

This is weird:

--- 
old/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp 
2019-07-16 22:11:30.200258978 +0300
+++ 
new/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp 
2019-07-16 22:11:29.867374851 +0300

@@ -127,7 +127,7 @@
    }

    void LinuxPlatform::SetCurrentDirectory(TString Value) {
- chdir(PlatformString(Value).toPlatformString());
+    int ignored = 
chdir(PlatformString(Value).toPlatformString());

    }

Firstly it does not fix the problem: you've gone from an unused 
value to an
unused variable. Secondly, we ignore the return value of printf 
all the time;

what's different about this one?
chdir() is __wur, which is 
/usr/include/aarch64-linux-gnu/sys/cdefs.h:#

define __wur __attribute_warn_unused_result__

OK, got it.


In some places this warning is disabled (CoreLibraries.gmk,
Awt2dLibraries.gmk).

It would be more correct to handle the result here as chdir 
might not

succeeded in fact.
Very much so. We shouldn't try to "shut up the compiler" in this 
way.Andrew Haley 

I created a separate bug about chdir() and write() usages:
https://bugs.openjdk.java.net/browse/JDK-8228402

Does it look good to silence warnings for platforms support changes?

I think we should fix the bugs, not silence the warnings.


One
more option could be to exclude related changes from current review.

OK.

Review Request: JDK-8209005: Lookup.unreflectSpecial fails for default methods when Lookup.findSpecial works

[Mandy Chung]

This patch fixes Lookup.unreflectSpecial to pass the declaring class of 
Method being unreflected (rather than null) so that it can accurately 
check if the special caller class is either the lookup class or a 
superinterface of the declaring class.


Webrev:
http://cr.openjdk.java.net/~mchung/jdk14/8209005/webrev.00/index.html

The test runs in both unnamed module and named module to cover 
JDK-8209078 which has been resolved by JDK-8173978.


thanks
Mandy

Re: [14] RFR: 8212970: TZ database in "vanguard" format support

[naoto . sato]

Hi Roger,

On 7/25/19 7:47 AM, Roger Riggs wrote:

Hi Naoto,

TestZoneInfo310.java:
With the composition of the tzdir path up and over to the make directory 
for the tzdir

it might be useful to do an explicit check that the directory exists.
That way if the directory structure on the build side changes,
there will be a test failure makine it obvious that the dependency has 
changed.


If the input tz data files, either in "test" tree or "make" tree, cannot 
be located, the test will fail which effectively reports if there is a 
repo structure change. So I believe it is ok as it is.


Aside from it, the latest changes to eliminate the duplicates caused 
that regression test fail. The reason was that there were actually two 
"jdk11_backward" data files each in "tzdata" and "tzdata_jdk" test 
directories, and the contents differ! I am not sure the reason why there 
are two files this way (seems to be so for years), so I reverted that 
exact file as before. Here is the webrev reflected that:


http://cr.openjdk.java.net/~naoto/8212970/webrev.12/

Naoto



Looks fine.

Thanks, Roger



On 7/24/19 6:24 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for the review.

On 7/24/19 2:57 PM, Joe Wang wrote:

Hi Naoto,

The method findNegativeSavings method in TzdbZoneRulesProvider.java 
states that it "Find the minimum negative savings". While the result 
is correct since the rules all have the same value for SAVE, I wonder 
if that's ideal conceptually. Given a start LDT, shouldn't it be 
looking for the SAVE in the exact (narrower) date range (e.g. 1981 - 
1989 vs 1981 - max)?.


I believe it is working as such. The end year is retrieved within the 
method (line 879) and only the minimum negative saving values within 
the window is filtered.




NegativeDSTTest verifies the tzdata, that is the adjusted data after 
import, is that correct? I wonder a comment and a bit of details in 
the test summary would be helpful since there is no negative data in 
the test itself.


Good point. It is confusing. I supplied summary text in the test (also 
the similar line in TestZoneRules.java)


Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8212970/webrev.11/

Naoto


Best,
Joe

On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following enhancement:

https://bugs.openjdk.java.net/browse/JDK-8212970

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8212970/webrev.09/

This change aims to support the "vanguard" IANA time zone data 
format, which uses the negative savings and transition time beyond a 
day period. The change basically translates those negative savings 
and transition times, such as 25:00, into the ones that the current 
JDK recognizes, then produces the data file "tzdb.dat" at the build 
time. At the run time, the data file is read and interpreted as 
before. This way the produced tzdb.dat is compatible with the prior 
JDK releases so that the TZ Updater can also distribute it as a time 
zone update.


I have also refactored redundant copy of ZoneRules file in the build 
directory, by dynamically importing the file under src. Thus some 
build related files are modified. I am hoping folks on the build-dev 
can review those changes.


Naoto

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

[Brian Burkhalter]

Hi Pavel,

> On Jul 25, 2019, at 12:43 PM, Pavel Rappo  wrote:
> 
> Thanks a lot for picking up a bug I filled back in 2015. This looks like a 
> good
> cleanup!

Thanks for looking at it.

> I'm a bit concerned though with the added `finally` block in L98. Could that
> lead to a subtle behavioral change for (an unlikely) case where a `read` 
> method
> exhausts the current stream, then tries to close it, fails (`close` throws
> IOException) and then jumps over to the next stream instead of staying on the
> current one?
> 
> Previously, `read` would be tripping over a closed stream forever.

This concern would be fixed by changing the proposed patch as

--- a/src/java.base/share/classes/java/io/SequenceInputStream.java
+++ b/src/java.base/share/classes/java/io/SequenceInputStream.java
@@ -91,13 +91,10 @@
  * Continues reading in the next stream if an EOF is reached.
  */
 final void nextStream() throws IOException {
-try {
-if (in != null) {
-in.close();
-}
-} finally {
-peekNextStream();
+if (in != null) {
+in.close();
 }
+peekNextStream();
 }
 
 private void peekNextStream() {
@@ -233,6 +230,7 @@
 } else {
 ioe.addSuppressed(e);
 }
+peekNextStream();
 }
 } while (in != null);
 if (ioe != null) {

That is to say reverting the nextStream() change and adding peekNextStream() in 
the catch branch of close(). I actually had it this way in a version I did not 
post.

The scenario you suggest however would seem to be somewhat of a coding error by 
the caller. I would think that once a sequence stream were created the 
component streams should not be used.

> Theoretically speaking, we might have a case of a "non-sticky" error in the
> InputStream. Try to read, fail, try to read again -- you might get lucky.

Thanks,

Brian

Re: 8078891: java.io.SequenceInputStream.close is not atomic and not idempotent

[Pavel Rappo]

Brian,

Thanks a lot for picking up a bug I filled back in 2015. This looks like a good
cleanup!

I'm a bit concerned though with the added `finally` block in L98. Could that
lead to a subtle behavioral change for (an unlikely) case where a `read` method
exhausts the current stream, then tries to close it, fails (`close` throws
IOException) and then jumps over to the next stream instead of staying on the
current one?

Previously, `read` would be tripping over a closed stream forever. 

Theoretically speaking, we might have a case of a "non-sticky" error in the
InputStream. Try to read, fail, try to read again -- you might get lucky.

Thanks,
-Pavel

> On 24 Jul 2019, at 01:09, Brian Burkhalter  
> wrote:
> 
> https://bugs.openjdk.java.net/browse/JDK-8078891
> http://cr.openjdk.java.net/~bpb/8078891/webrev.00/
> 
> Ensure that SequenceInputStream closes all component streams.
> 
> Thanks,
> 
> Brian

Re: [14] RFR: 8212970: TZ database in "vanguard" format support

[Erik Joelsson]

Build change looks good.

/Erik

On 2019-07-24 15:24, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for the review.

On 7/24/19 2:57 PM, Joe Wang wrote:

Hi Naoto,

The method findNegativeSavings method in TzdbZoneRulesProvider.java 
states that it "Find the minimum negative savings". While the result 
is correct since the rules all have the same value for SAVE, I wonder 
if that's ideal conceptually. Given a start LDT, shouldn't it be 
looking for the SAVE in the exact (narrower) date range (e.g. 1981 - 
1989 vs 1981 - max)?.


I believe it is working as such. The end year is retrieved within the 
method (line 879) and only the minimum negative saving values within 
the window is filtered.




NegativeDSTTest verifies the tzdata, that is the adjusted data after 
import, is that correct? I wonder a comment and a bit of details in 
the test summary would be helpful since there is no negative data in 
the test itself.


Good point. It is confusing. I supplied summary text in the test (also 
the similar line in TestZoneRules.java)


Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8212970/webrev.11/

Naoto


Best,
Joe

On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following enhancement:

https://bugs.openjdk.java.net/browse/JDK-8212970

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8212970/webrev.09/

This change aims to support the "vanguard" IANA time zone data 
format, which uses the negative savings and transition time beyond a 
day period. The change basically translates those negative savings 
and transition times, such as 25:00, into the ones that the current 
JDK recognizes, then produces the data file "tzdb.dat" at the build 
time. At the run time, the data file is read and interpreted as 
before. This way the produced tzdb.dat is compatible with the prior 
JDK releases so that the TZ Updater can also distribute it as a time 
zone update.


I have also refactored redundant copy of ZoneRules file in the build 
directory, by dynamically importing the file under src. Thus some 
build related files are modified. I am hoping folks on the build-dev 
can review those changes.


Naoto

RFR: 8228623 : Update copyright year to 2019 for several java properties file

[li . jiang]

Hi,

Please review this trivial change to correct the copyright year in 
several java properties file. I updated the copyright year in English, 
Simplified Chinese and Japanese resource files.


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

Webrev:
http://cr.openjdk.java.net/~ljiang/8228623/webrev/

Thanks,
Leo

Re: RFR: JDK-8227312: Remove pkg bundle from DMG image.

[Kevin Rushforth]

Looks fine.

-- Kevin

On 7/25/2019 6:24 AM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


[1] https://bugs.openjdk.java.net/browse/JDK-8227312

[2] http://cr.openjdk.java.net/~herrick/8227312/webrev.01/

/Andy

Re: RFR: JDK-8227684 : jpackage must handle win32 mangled names in jli.dll

[Kevin Rushforth]

It looks fine.

-- Kevin


On 7/25/2019 6:20 AM, Andy Herrick wrote:

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).



[1] https://bugs.openjdk.java.net/browse/JDK-8227684

[2] http://cr.openjdk.java.net/~herrick/8227684/webrev.01/

Submitted by: Robert Lichtenberger

/Andy

Re: [14] RFR: 8212970: TZ database in "vanguard" format support

[Roger Riggs]

Hi Naoto,

TestZoneInfo310.java:
With the composition of the tzdir path up and over to the make directory 
for the tzdir

it might be useful to do an explicit check that the directory exists.
That way if the directory structure on the build side changes,
there will be a test failure makine it obvious that the dependency has 
changed.


Looks fine.

Thanks, Roger



On 7/24/19 6:24 PM, naoto.s...@oracle.com wrote:

Hi Joe,

Thank you for the review.

On 7/24/19 2:57 PM, Joe Wang wrote:

Hi Naoto,

The method findNegativeSavings method in TzdbZoneRulesProvider.java 
states that it "Find the minimum negative savings". While the result 
is correct since the rules all have the same value for SAVE, I wonder 
if that's ideal conceptually. Given a start LDT, shouldn't it be 
looking for the SAVE in the exact (narrower) date range (e.g. 1981 - 
1989 vs 1981 - max)?.


I believe it is working as such. The end year is retrieved within the 
method (line 879) and only the minimum negative saving values within 
the window is filtered.




NegativeDSTTest verifies the tzdata, that is the adjusted data after 
import, is that correct? I wonder a comment and a bit of details in 
the test summary would be helpful since there is no negative data in 
the test itself.


Good point. It is confusing. I supplied summary text in the test (also 
the similar line in TestZoneRules.java)


Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8212970/webrev.11/

Naoto


Best,
Joe

On 7/23/19 3:15 PM, naoto.s...@oracle.com wrote:

Hi,

Please review the fix to the following enhancement:

https://bugs.openjdk.java.net/browse/JDK-8212970

The proposed changeset is located at:

https://cr.openjdk.java.net/~naoto/8212970/webrev.09/

This change aims to support the "vanguard" IANA time zone data 
format, which uses the negative savings and transition time beyond a 
day period. The change basically translates those negative savings 
and transition times, such as 25:00, into the ones that the current 
JDK recognizes, then produces the data file "tzdb.dat" at the build 
time. At the run time, the data file is read and interpreted as 
before. This way the produced tzdb.dat is compatible with the prior 
JDK releases so that the TZ Updater can also distribute it as a time 
zone update.


I have also refactored redundant copy of ZoneRules file in the build 
directory, by dynamically importing the file under src. Thus some 
build related files are modified. I am hoping folks on the build-dev 
can review those changes.


Naoto

Benchmarking of new optimized Dual-Pivot Quicksort

[Vladimir Yaroslavskiy]

Hi all,
With the help of Laurent Bourges I run benchmarking of new optimized Dual-Pivot 
Quicksort
and summarized results. The tests were run for all types (int / long / ... / 
double), for both types:
sequential and parallel sorting, for small, medium and large sizes. The 
comparison was done
on several data types, such as: random, period, sorted, equal, stagger, shuffle.
Here is the summary of total gains. The gain is defined as (jdk_time - 
dpqs_time) / jdk_time,
where jdk_time is sorting time by the current jdk14 and dpqs_time is sorting 
time by new
optimized Dual-Pivot Quicksort. To get stable and reproducible results, min 
time of many
invocations is taken.
You can find all detailed results there
https://github.com/iaroslavski/sorting/tree/master/benchmarking/results  

Sources of benchmarking tests are there
https://github.com/iaroslavski/sorting/tree/master/benchmarking/sources

You can see not good performance for float / double types (parallel sorting).
This behavior can be explained by existing bug in jdk, when NaNs and -0.0s
are not processed properly. New sorting has total losses for small float / 
double
arrays, few percents only. For all other cases new optimized sorting is winner.


[int]
sequential, Length = 150, Gain: 0.15
sequential, Length = 3, Gain: 0.28
sequential, Length = 100, Gain: 0.31
parallel 8, Length = 150, Gain: 0.14
parallel 8, Length = 3, Gain: 0.15
parallel 8, Length = 100, Gain: 0.14
[long]
sequential, Length = 150, Gain: 0.12
sequential, Length = 3, Gain: 0.23
sequential, Length = 100, Gain: 0.32
parallel 8, Length = 150, Gain: 0.11
parallel 8, Length = 3, Gain: 0.16
parallel 8, Length = 100, Gain: 0.24
parallel 88 processors, Length = 100, Gain: 0.05
[byte]
sequential, Length = 150, Gain: 0.06
sequential, Length = 3, Gain: 0.36
sequential, Length = 100, Gain: 0.37
parallel 8, Length = 150, Gain: 0.13
parallel 8, Length = 3, Gain: 0.73
parallel 8, Length = 100, Gain: 0.65
[char]
sequential, Length = 150, Gain: 0.10
sequential, Length = 3, Gain: 0.00
sequential, Length = 100, Gain: 0.17
parallel 8, Length = 150, Gain: 0.10
parallel 8, Length = 3, Gain: 0.33
parallel 8, Length = 100, Gain: 0.62
[short]
sequential, Length = 150, Gain: 0.14
sequential, Length = 3, Gain: 0.10
sequential, Length = 100, Gain: 0.18
parallel 8, Length = 150, Gain: 0.13
parallel 8, Length = 3, Gain: 0.41
parallel 8, Length = 100, Gain: 0.65
[float]
sequential, Length = 150, Gain: -0.02
sequential, Length = 3, Gain: 0.21
sequential, Length = 100, Gain: 0.24
parallel 8, Length = 150, Gain: -0.05
parallel 8, Length = 3, Gain: -0.04
parallel 8, Length = 100, Gain: -0.12
[double]
sequential, Length = 150, Gain: -0.03
sequential, Length = 3, Gain: 0.19
sequential, Length = 100, Gain: 0.23
parallel 8, Length = 150, Gain: -0.05
parallel 8, Length = 3, Gain: 0.05
parallel 8, Length = 100, Gain: 0.15
Thank you,
Vladimir

RE: RFR: 8228482: fix xlc16/xlclang comparison of distinct pointer types and string literal conversion warnings

[Baesken, Matthias]

Thanks Martin .
May I get a second review ?

Best regards, Matthias


From: Doerr, Martin
Sent: Mittwoch, 24. Juli 2019 12:14
To: Baesken, Matthias ; 
'hotspot-...@openjdk.java.net' ; 
core-libs-dev@openjdk.java.net; net-...@openjdk.java.net
Cc: 'ppc-aix-port-...@openjdk.java.net' 
Subject: RE: RFR: 8228482: fix xlc16/xlclang comparison of distinct pointer 
types and string literal conversion warnings

Hi Matthias,

I wouldn’t say „looks good”, but I think it’s the right thing to do 
The type casts look correct to fit to the AIX headers.

libodm_aix:
Good. Maybe we should open a new issue for freeing what’s returned by 
odm_set_path and we could also remove the AIX 5 support.

NetworkInterface.c:
Strange, but ok. Should be reviewed by somebody else in addition.

Other files:
No comments.

Best regards,
Martin


From: ppc-aix-port-dev 
mailto:ppc-aix-port-dev-boun...@openjdk.java.net>>
 On Behalf Of Baesken, Matthias
Sent: Dienstag, 23. Juli 2019 17:15
To: 'hotspot-...@openjdk.java.net' 
mailto:hotspot-...@openjdk.java.net>>; 
core-libs-dev@openjdk.java.net; 
net-...@openjdk.java.net
Cc: 'ppc-aix-port-...@openjdk.java.net' 
mailto:ppc-aix-port-...@openjdk.java.net>>
Subject: RFR: 8228482: fix xlc16/xlclang comparison of distinct pointer types 
and string literal conversion warnings

Hello please review this patch .

It fixes a couple of  xlc16/xlclang warnings  , especially  comparison of 
distinct pointer types and string literal conversion warnings  .

When building with xlc16/xlclang, we still have a couple of warnings that have 
to be fixed :
warning: ISO C++11 does not allow conversion from string literal to 'char *' 
[-Wwritable-strings]
for example :
/nightly/jdk/src/hotspot/os/aix/libodm_aix.cpp:81:18: warning: ISO C++11 does 
not allow conversion from string literal to 'char *' [-Wwritable-strings]
  odmWrapper odm("product", "/usr/lib/objrepos"); // could also use "lpp"
 ^
/nightly/jdk/src/hotspot/os/aix/libodm_aix.cpp:81:29: warning: ISO C++11 does 
not allow conversion from string literal to 'char *' [-Wwritable-strings]
  odmWrapper odm("product", "/usr/lib/objrepos"); // could also use "lpp"
^

warning: comparison of distinct pointer types, for example :

/nightly/jdk/src/java.desktop/aix/native/libawt/porting_aix.c:50:14: warning: 
comparison of distinct pointer types ('void *' and 'char *') 
[-Wcompare-distinct-pointer-types]
addr < (((char*)p->ldinfo_textorg) + p->ldinfo_textsize)) {
 ^ ~



Bug/webrev :


https://bugs.openjdk.java.net/browse/JDK-8228482

http://cr.openjdk.java.net/~mbaesken/webrevs/8228482.1/


Thanks, Matthias

RFR: JDK-8227312: Remove pkg bundle from DMG image.

[Andy Herrick]

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).


[1] https://bugs.openjdk.java.net/browse/JDK-8227312

[2] http://cr.openjdk.java.net/~herrick/8227312/webrev.01/

/Andy

RFR: JDK-8227684 : jpackage must handle win32 mangled names in jli.dll

[Andy Herrick]

Please review the jpackage fix for bug [1] at [2].

This is a fix for the JDK-8200758-branch branch of the open sandbox 
repository (jpackage).



[1] https://bugs.openjdk.java.net/browse/JDK-8227684

[2] http://cr.openjdk.java.net/~herrick/8227684/webrev.01/

Submitted by: Robert Lichtenberger

/Andy

Re: RFR (S): JDK-8222778: Packaging Tool (JEP 343) on Linux/AArch64

[Dmitry Chuyko]

I realized we also need better detection of arch for deb control file. 
It can be done by using 'dpkg --print-architecture' on host. 
LinuxDebBundler was corrected.


New webrev: http://cr.openjdk.java.net/~dchuyko/8222778/webrev.04/

Patch was re-tested together with webrev.02 from JDK-8228402.

-Dmitry

On 7/25/19 12:56 AM, Alexander Matveev wrote:

Looks good.

On 7/24/2019 7:30 AM, Dmitry Chuyko wrote:
Webrev without chdir() and write() changes: 
http://cr.openjdk.java.net/~dchuyko/8222778/webrev.03/


When it is applied together with webrev.01 [1] from JDK-8228402, 
sandbox is compiled and passes same jpackage tests on x86, aarch64 
and other platforms.


-Dmitry

[1] http://cr.openjdk.java.net/~herrick/8228402/

On 7/19/19 11:31 AM, Andrew Haley wrote:

On 7/19/19 12:36 AM, Dmitry Chuyko wrote:

On 7/18/19 11:57 AM, Andrew Haley wrote:

Hi,

On 7/17/19 10:19 PM, Dmitry Chuyko wrote:

On 7/17/19 11:52 AM, Andrew Haley wrote:

This is weird:

--- 
old/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp 
2019-07-16 22:11:30.200258978 +0300
+++ 
new/src/jdk.jpackage/linux/native/libapplauncher/LinuxPlatform.cpp 
2019-07-16 22:11:29.867374851 +0300

@@ -127,7 +127,7 @@
    }

    void LinuxPlatform::SetCurrentDirectory(TString Value) {
-    chdir(PlatformString(Value).toPlatformString());
+    int ignored = chdir(PlatformString(Value).toPlatformString());
    }

Firstly it does not fix the problem: you've gone from an unused 
value to an
unused variable. Secondly, we ignore the return value of printf 
all the time;

what's different about this one?
chdir() is __wur, which is 
/usr/include/aarch64-linux-gnu/sys/cdefs.h:#

define __wur __attribute_warn_unused_result__

OK, got it.


In some places this warning is disabled (CoreLibraries.gmk,
Awt2dLibraries.gmk).

It would be more correct to handle the result here as chdir might 
not

succeeded in fact.
Very much so. We shouldn't try to "shut up the compiler" in this 
way.Andrew Haley 

I created a separate bug about chdir() and write() usages:
https://bugs.openjdk.java.net/browse/JDK-8228402

Does it look good to silence warnings for platforms support changes?

I think we should fix the bugs, not silence the warnings.


One
more option could be to exclude related changes from current review.

OK.

RFR CSR JDK-8202555: Double.toString(double) sometimes produces incorrect results

[Raffaello Giulietti]

Hi folks,

as nothing has substantially moved towards approval of the patch [1] and 
the corresponding CSR [2], let's split the issue into its constituents.


The CSR is currently finalized and needs a further review for approval. 
This requires much less time than a review of the implementation [1].


The CSR has been carefully written to match the observed behavior of the 
current OpenJDK 12 as close as possible while aiming at being more clear 
and 100% rigorous. It can be approved independently of the underlying 
implementation.


So please review and approve the CSR [2], regardless of the proposed 
improved implementation [1], which can be tackled later.



Greetings
Raffaello



[1] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-April/059783.html

[2] https://bugs.openjdk.java.net/browse/JDK-8202555
[3] https://drive.google.com/open?id=1KLtG_LaIbK9ETXI290zqCxvBW94dj058