Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation

[Mandy Chung]

On 11/7/19 1:45 PM, 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).


This changes the module name, and base package name from jdk.jpackage 
to jdk.incubator.jpackage.


This removes the "provides" statement in module-info so 
jdk.incubator.jpackage can not directly be discovered as a 
ToolProvider, but it also exports jdk.incubator.jpackage which now 
contains ToolProviderFactory.java which can be used for the same purpose.


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

[2] https://cr.openjdk.java.net/~herrick/8233636/webrev.02


make/common/Modules.gmk
src/java.base/share/classes/module-info.java
   - it'd be good to keep the list in alphabetical order

jdk.incubator.jpackage.ToolProviderFactory is exported and needs the 
specification and javadoc.


  34 private static ToolProvider provider = new JPackageToolProvider();

Nit: this can be final field.

Mandy

Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation

[Alexander Matveev]

Hi Andy,

Agree with Alexey comments. Otherwise looks good.

Thanks,
Alexander

On 11/7/2019 2:44 PM, Andy Herrick wrote:


On 11/7/2019 5:38 PM, Alexey Semenyuk wrote:
I guess the link for the review is 
https://cr.openjdk.java.net/~herrick/8233636/webrev.02



sry: yes webrev is http://cr.openjdk.java.net/~herrick/8233636/webrev.02
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.sdiff.html:50 


I'd suggest to replace
---
if (name.equals("jpackage"))
---

sure, I can do that.


with more robust
---
if (this == JPACKAGE)
---

http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/share/jdk/jpackage/tests/BasicTest.java.sdiff.html: 


Did you drop JPackageCommand.filterOutput() calls intentionally?

The rest looks good.

- Alexey

On 11/7/2019 4:45 PM, 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).


This changes the module name, and base package name from 
jdk.jpackage to jdk.incubator.jpackage.


This removes the "provides" statement in module-info so 
jdk.incubator.jpackage can not directly be discovered as a 
ToolProvider, but it also exports jdk.incubator.jpackage which now 
contains ToolProviderFactory.java which can be used for the same 
purpose.


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

[2] https://cr.openjdk.java.net/~herrick/8233636/webrev-02

/Andy

RFR: 8231863: Crash if classpath is read from @argument file and the main gets option argument

[Henry Jen]

Hi,

Please review the webrev[1], contributed by Mat Carter. You can find the bug 
details at JBS[2]. I have reviewed and tested the fix, I still need an official 
review before I can push this.

Cheers,
Henry


[1] http://cr.openjdk.java.net/~henryjen/jdk/8231863.0/webrev/
[2] https://bugs.openjdk.java.net/browse/JDK-8231863


> On Nov 4, 2019, at 3:40 PM, Mat Carter  wrote:
> 
> Thanks Henry
> 
> I'm working on a test, do you want me to share directly with you (incl. 
> questions) or continue with comms through the mailing list
> 
> Cheers
> Mat
> 
> From: Henry Jen 
> Sent: Monday, November 4, 2019 12:47 PM
> To: Mat Carter 
> Cc: core-libs-dev@openjdk.java.net 
> Subject: Re: JDK-8231863 bug fix candidate
>  
> Hi Mat,
> 
> I’ll sponsor this fix, let me know if you are coming up with test and I’ll 
> work with you to get your patch committed.
> 
> Cheers,
> Henry
> 
> 
> > On Nov 4, 2019, at 9:21 AM, Henry Jen  wrote:
> > 
> > The fix is on the spot, good catch.
> > 
> > As the test, test/jdk/tools/launcher/ArgsFileTest.java is testing the file 
> > expansions, we can add a test case there to have an argument file not have 
> > newline at the end, and check that we get correct arguments.
> > 
> > Cheers,
> > Henry
> > 
> > 
> >> On Nov 1, 2019, at 7:06 AM, Mat Carter  
> >> wrote:
> >> 
> >> Hello core-libs-dev
> >> 
> >> I'm a VM engineer at Microsoft, now that we've signed the OCA (news of 
> >> which was shared by Bruno Borges in the discuss mailing list) I wanted to 
> >> pick up an issue in order to gain familiarity with the change process
> >> 
> >> After reproducing the error reported in "JDK-8231863: Crash if classpath 
> >> is read from @argument file and the main gets option argument" 
> >> (https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.openjdk.java.net%2Fbrowse%2FJDK-8231863data=02%7C01%7CMatthew.Carter%40microsoft.com%7C657f0afad82b4d31670a08d761682b0b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637085026486748197sdata=19S%2BLGCPaoy0h2YmbKrKfsC21d6B43rITkZhnAKyuHw%3Dreserved=0),
> >>  I've identified the root cause and a candidate fix.
> >> 
> >> As such I'd like to pick this issue up, assuming its current state of 
> >> unassigned still holds what's the process of having it assigned to me or 
> >> having it somehow reserved as I don't want to cause unnecessary duplicated 
> >> work.
> >> 
> >> As well as validating the fix in the debugger on WIndows and Linux, I've 
> >> run the AdoptOpenJDK sanity tests on Mac, Windows and Linux for both tip 
> >> and 11u.
> >> 
> >> I'm currently figuring out where/how to write regression tests for this 
> >> case; input from this mailing list would be welcomed (examples of tests 
> >> for similar bugs etc)
> >> 
> >> This error occurs on all platforms (tip + 11u) but only results in a crash 
> >> on Windows in java_md.c due to that platforms dependency on 
> >> JLI_GetAppArgIndex().  Note that while the error is 100% reproducible, the 
> >> crash only occurs <10%.  The following change fixes that problem by 
> >> passing in the final token fragment from the argument file into the state 
> >> machine via checkArg()
> >> 
> >> --- a/src/java.base/share/native/libjli/args.c
> >> +++ b/src/java.base/share/native/libjli/args.c
> >> @@ -337,7 +337,9 @@ static JLI_List readArgFile(FILE *file) {
> >> // remaining partial token
> >> if (ctx.state == IN_TOKEN || ctx.state == IN_QUOTE) {
> >> if (ctx.parts->size != 0) {
> >> -JLI_List_add(rv, JLI_List_combine(ctx.parts));
> >> +token = JLI_List_combine(ctx.parts);
> >> +checkArg(token);
> >> +JLI_List_add(rv, token);
> >> }
> >> }
> >> JLI_List_free(ctx.parts);
> >> 
> >> The fix tackles the memory corruption indirectly.  Further preventative 
> >> changes could be made in java_md.c/CreateApplicationArgs to avoid future 
> >> memory corruptions (caused by JLI_GetAppArgIndex() returning -1 in this 
> >> case) ; but I felt that that handling those error cases ran secondary to 
> >> addressing the bug in the argument file parsing code.
> >> 
> >> Cheers
> >> Mat
> > 
> 

Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation

[Andy Herrick]

On 11/7/2019 5:38 PM, Alexey Semenyuk wrote:
I guess the link for the review is 
https://cr.openjdk.java.net/~herrick/8233636/webrev.02



sry: yes webrev is http://cr.openjdk.java.net/~herrick/8233636/webrev.02
http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.sdiff.html:50 


I'd suggest to replace
---
if (name.equals("jpackage"))
---

sure, I can do that.


with more robust
---
if (this == JPACKAGE)
---

http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/share/jdk/jpackage/tests/BasicTest.java.sdiff.html: 


Did you drop JPackageCommand.filterOutput() calls intentionally?

The rest looks good.

- Alexey

On 11/7/2019 4:45 PM, 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).


This changes the module name, and base package name from jdk.jpackage 
to jdk.incubator.jpackage.


This removes the "provides" statement in module-info so 
jdk.incubator.jpackage can not directly be discovered as a 
ToolProvider, but it also exports jdk.incubator.jpackage which now 
contains ToolProviderFactory.java which can be used for the same 
purpose.


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

[2] https://cr.openjdk.java.net/~herrick/8233636/webrev-02

/Andy

Re: RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation

[Alexey Semenyuk]

I guess the link for the review is 
https://cr.openjdk.java.net/~herrick/8233636/webrev.02


http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/helpers/jdk/jpackage/test/JavaTool.java.sdiff.html:50 


I'd suggest to replace
---
if (name.equals("jpackage"))
---

with more robust
---
if (this == JPACKAGE)
---

http://cr.openjdk.java.net/~herrick/8233636/webrev.02/test/jdk/tools/jpackage/share/jdk/jpackage/tests/BasicTest.java.sdiff.html: 


Did you drop JPackageCommand.filterOutput() calls intentionally?

The rest looks good.

- Alexey

On 11/7/2019 4:45 PM, 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).


This changes the module name, and base package name from jdk.jpackage 
to jdk.incubator.jpackage.


This removes the "provides" statement in module-info so 
jdk.incubator.jpackage can not directly be discovered as a 
ToolProvider, but it also exports jdk.incubator.jpackage which now 
contains ToolProviderFactory.java which can be used for the same purpose.


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

[2] https://cr.openjdk.java.net/~herrick/8233636/webrev-02

/Andy

Re: RFR: JDK-8233117 Escape Sequences For Line Continuation and White Space (Preview)

[Jim Laskey]

Yep. :-) Too much juggling. Thank you.

> On Nov 7, 2019, at 5:23 PM, Brent Christian  
> wrote:
> 
> Should the new escapes be added to the table in the String.translateEscapes() 
> JavaDoc?
> 
> -Brent
> 
> On 11/7/19 6:22 AM, Jim Laskey wrote:
>> Please review the following code changes. Provides for the introduction of 
>> two new escape sequences \ and \s.  \ 
>> allows developers to express unwieldy string literals in a text block as a 
>> cluster of short single line segments. The second is to allow developers to 
>> express ASCII space, much like \t for tab. The changes are primarily in the 
>> compiler, with some small changes in String::translateEscapes. Small 
>> changeset overall.
>> Thank you.
>> -- Jim
>> webrev: http://cr.openjdk.java.net/~jlaskey/8233116/webrev/index.html 
>> 
>> jbs: https://bugs.openjdk.java.net/browse/JDK-8233116 
>> 
>> csr: https://bugs.openjdk.java.net/browse/JDK-8233117 
>> 

Re: JDK 14 RFR of JDK-8233452: java.math.BigDecimal.sqrt() with RoundingMode.FLOOR results in incorrect result

[Brian Burkhalter]

Hi Joe,

> On Nov 5, 2019, at 6:04 PM, Joe Darcy  wrote:
> 
> Please review the changes to fix
> 
> JDK-8233452: java.math.BigDecimal.sqrt() with RoundingMode.FLOOR results 
> in incorrect result
> http://cr.openjdk.java.net/~darcy/8233452.0/ 
> 
> 
> Some background on the problem and fix.
> 
> The core of the BigDecimal.sqrt method is a Newton-Raphson loop.
> […]
> The Newton iteration reduces the error at each step and, as currently 
> written, it can settle on a value like 2, which is closer to the actual 
> answer, but *incorrect* according to the requested rounding mode.
> 
> There are a few ways to fix this one. One is to run the Newton loop to a 
> higher-precision where these finer distinctions can be teased out. […]
> 
> Another approach is to detect when the result is too large/too small and then 
> subtract/add an ulp as a fix-up. This is the approach taken for 
> BigDecimal.sqrt. […]

This implementation change looks reasonable to me.

> An approach not explored for this fix would be to arrange for the iteration 
> to start from the "right" side of the number line depending on the rounding 
> mode and then monotonically increase/decrease to approach the square root. 
> […] I believe this is technically possible, but would require some additional 
> analysis to setup.

This approach seems more appealing and could perhaps be investigated at a later 
date.

In the test I think you want to replace “down” with “up" at line 238.

Brian

RFR: JDK-8233636: Make jpackage an incubator and remove tool provider implementation

[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).


This changes the module name, and base package name from jdk.jpackage to 
jdk.incubator.jpackage.


This removes the "provides" statement in module-info so 
jdk.incubator.jpackage can not directly be discovered as a ToolProvider, 
but it also exports jdk.incubator.jpackage which now contains 
ToolProviderFactory.java which can be used for the same purpose.


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

[2] https://cr.openjdk.java.net/~herrick/8233636/webrev-02

/Andy

Re: RFR: JDK-8233117 Escape Sequences For Line Continuation and White Space (Preview)

[Brent Christian]

Should the new escapes be added to the table in the 
String.translateEscapes() JavaDoc?


-Brent

On 11/7/19 6:22 AM, Jim Laskey wrote:

Please review the following code changes. Provides for the introduction of two new escape 
sequences \ and \s.  \ allows developers to 
express unwieldy string literals in a text block as a cluster of short single line 
segments. The second is to allow developers to express ASCII space, much like \t for tab. 
The changes are primarily in the compiler, with some small changes in 
String::translateEscapes. Small changeset overall.

Thank you.

-- Jim


webrev: http://cr.openjdk.java.net/~jlaskey/8233116/webrev/index.html 


jbs: https://bugs.openjdk.java.net/browse/JDK-8233116 

csr: https://bugs.openjdk.java.net/browse/JDK-8233117 

Re: [14] RFR: 8232871: Host Locale Provider on Mac does not return translated values of Japanese calendar

[Brent Christian]

Looks good.  -B

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


Here is the updated webrev:

https://cr.openjdk.java.net/~naoto/8232871/webrev.01/

Re: RFR [14/java.xml] 8233686: XML transformer uses excessive amount of memory

[Lance Andersen]

Looks OK Joe.  

> On Nov 7, 2019, at 2:01 PM, Joe Wang  wrote:
> 
> Please review a quick fix that reduces unnecessary object allocations.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8233686
> webrev: http://cr.openjdk.java.net/~joehw/jdk14/8233686/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 

RFR [14/java.xml] 8233686: XML transformer uses excessive amount of memory

[Joe Wang]

Please review a quick fix that reduces unnecessary object allocations.

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

Thanks,
Joe

Re: RFR: JDK-8232684: Make switch expressions final

[Vladimir Kozlov]

HotSpot tests changes (using Thread.yield()) look good.

Thanks,
Vladimir

On 11/5/19 1:50 AM, Jan Lahoda wrote:
I've missed updates to some hotspot and jdk tests in the first patch. The problem are unqualified 
invocations of Thread.yield(), which are no longer allowed (from the spec: JLS 3.9 Keywords: "All 
invocations of a method called yield must be qualified so as to be distinguished from a yield 
statement."). Therefore, these invocations need to be changed to qualified invocations (i.e. 
Thread.yield()). Updates to the hotspot and jdk tests are here:

http://cr.openjdk.java.net/~jlahoda/8232684/webrev.delta.00.01/

Full updated webrev, with changes to both javac and the tests, is here:
http://cr.openjdk.java.net/~jlahoda/8232684/webrev.01/

How does that look?

Thanks,
     Jan

On 21. 10. 19 16:17, Maurizio Cimadamore wrote:

Looks generally good -  went through the test updates one by one and they look 
ok, except this:

http://cr.openjdk.java.net/~jlahoda/8232684/webrev.00/test/langtools/tools/jdeps/listdeps/ListModuleDeps.java.udiff.html 



Which you explained to me offline (we need to change this code every time the compiler stops using 
the @Preview annotation - ugh). Nothing specific to this webrev, so I approve.


Maurizio

On 21/10/2019 14:49, Jan Lahoda wrote:

Hi,

As part of preparation for proposing JEP 361: Switch Expressions (Standard) to target, I would 
like to ask for a review of the patch to make switch expression a non-preview feature in javac:

http://cr.openjdk.java.net/~jlahoda/8232684/webrev.00/

The patch basically removes the feature from the list of preview features, updates test to this 
new state (removes --enable-preview from associated tests, and adjusts their expected output), 
and removes the @PreviewFeature annotation and associated text from the javadoc of the Trees API.


I also would like to ask for a review for the CSR associated with that:
https://bugs.openjdk.java.net/browse/JDK-8232685

Reviews/comments on either of these would be very welcome!

JBS: https://bugs.openjdk.java.net/browse/JDK-8232684

Thanks!

Jan

RFR: JDK-8233117 Escape Sequences For Line Continuation and White Space (Preview)

[Jim Laskey]

Please review the following code changes. Provides for the introduction of two 
new escape sequences \ and \s.  \ allows 
developers to express unwieldy string literals in a text block as a cluster of 
short single line segments. The second is to allow developers to express ASCII 
space, much like \t for tab. The changes are primarily in the compiler, with 
some small changes in String::translateEscapes. Small changeset overall.

Thank you.

-- Jim


webrev: http://cr.openjdk.java.net/~jlaskey/8233116/webrev/index.html 


jbs: https://bugs.openjdk.java.net/browse/JDK-8233116 

csr: https://bugs.openjdk.java.net/browse/JDK-8233117