Re: [PATCH] 8218268: Javac treats Manifest Class-Path entries as Paths instead of URLs

2019-03-04 Thread Donald Kwakkel
> On 02/28/2019 01:06 PM, Alan Bateman wrote:
> > On 28/02/2019 20:58, Jonathan Gibbons wrote:
> >> Looking at the revised JAR specification, approved in [1], it is
> >> disappointing that the spec contains text which is specific to
> >> a JAR file being used in a classloader:
> >>
> >> |The resulting URLs are inserted into the search path of the class
> >> loader opening the context JAR immediately following the path of the
> >> context JAR. Any duplicated URLs are omitted.|
> >>
> >> That text would seem not to apply to javac and other tools that may wish
> >> to process the class files in a JAR file.
> > That should be fixed as it should apply at compile-time too.
> >
> > -Alan
>
> |Agreed it might be good to fix it if possible. All the preceding text
> is good, and can be handled by javac. The only questionable bit is the
> text "Any duplicated URLs are omitted" which could be moved up a bit in
> the spec to a new paragraph, and maybe rephrased to use "ignored"
> instead of "omitted". If that were done, all the stuff about class
> loaders could be taken as non-normative text.

So if I am correct the answer to Question 2 is: Yes, behavior must be the same.

What are the next steps to take for this? And can someone also answer my
other questions?:

Question 1: Where can I find the tests for these changes?
Question 2: Where should the common code for this be located?
Question 3: Is it an idea to first implement below patch and backport
that, and in addition as new ticket implement the new behaviour also
for javac?
Question 4:Is this they way to do it, or is there a better way to
provide debug information to users running javac?


Re: RFR: JDK-8219678: CLI changes in jpackage

2019-03-04 Thread Andy Herrick




On 3/4/2019 8:46 PM, Alexander Matveev wrote:

Hi Andy,

http://cr.openjdk.java.net/~herrick/8219678/webrev.03/src/jdk.jpackage/share/classes/jdk/jpackage/internal/BundlerParamInfo.java.frames.html 

Line 32: Do we need this import? I do not see any changes which might 
require it.


Thanks - I had removed that line from several other files where I didn't 
think it was needed - and got an error.  I must have added it to this 
file when restoring it in the several other files.  I will fix.


/Andy


Otherwise looks fine.

Thanks,
Alexander

On 3/3/2019 7:48 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).


This change modifies the CLI and other behavior as described in [1]

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

[2] http://cr.openjdk.java.net/~herrick/8219678/


/Andy







Re: RFR: JDK-8219678: CLI changes in jpackage

2019-03-04 Thread Alexander Matveev

Hi Andy,

http://cr.openjdk.java.net/~herrick/8219678/webrev.03/src/jdk.jpackage/share/classes/jdk/jpackage/internal/BundlerParamInfo.java.frames.html
Line 32: Do we need this import? I do not see any changes which might 
require it.


Otherwise looks fine.

Thanks,
Alexander

On 3/3/2019 7:48 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).


This change modifies the CLI and other behavior as described in [1]

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

[2] http://cr.openjdk.java.net/~herrick/8219678/


/Andy





Re: RFR: JDK-8219679: Help text changes in jpackage

2019-03-04 Thread Andy Herrick
yes - I was thinking the list of extension need not be localized but 
didn't think of the "and" in it - will think of another way to do this.


/Andy


On 3/4/2019 6:56 PM, Jonathan Gibbons wrote:

Andy,

Is it something to be concerned about that you have English text 
directly in the source code, at CLIHelp.java, lines 57-65. 
Specifically, the repeated use of "and".


Is that something that should be localizable?

-- Jon

On 3/3/19 7:34 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-8219679

[2] http://cr.openjdk.java.net/~herrick/8219679/

/Andy





Re: RFR: JDK-8219679: Help text changes in jpackage

2019-03-04 Thread Jonathan Gibbons

Andy,

Is it something to be concerned about that you have English text 
directly in the source code, at CLIHelp.java, lines 57-65. Specifically, 
the repeated use of "and".


Is that something that should be localizable?

-- Jon

On 3/3/19 7:34 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-8219679

[2] http://cr.openjdk.java.net/~herrick/8219679/

/Andy



Re: RFR: JDK-8219679: Help text changes in jpackage

2019-03-04 Thread Alexander Matveev

Hi Andy,

http://cr.openjdk.java.net/~herrick/8219679/webrev.02/src/jdk.jpackage/share/classes/jdk/jpackage/internal/resources/HelpResources.properties.frames.html
Line 109: Missing ">" after main class?
Line 132: additionl - > additional

Otherwise looks fine.

Thanks,
Alexander

On 3/3/2019 7:34 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-8219679

[2] http://cr.openjdk.java.net/~herrick/8219679/

/Andy





Re: Proposal to enhance Stream.collect

2019-03-04 Thread August Nagro
I think documentation could help this (and maybe removing the two added
`of` helpers so an explicit collector must be created), but you're right
about the cost. If the consensus is not to change Collector, then maybe
Tagir's suggestion for internal-only change can be implemented?

On Mon, Mar 4, 2019 at 2:53 PM Brian Goetz  wrote:

> So, unfortunately I think this particular API evolution idiom is kind of
> questionable.  There is a new method, sizedSupplier, with a default that
> delegates to the unsized supplier.  But, that leaves implementors in a
> position where they don’t really know which one to implement, and the use
> of characteristics to hint at which one to call is kind of convoluted.
>
> You’re focused on the benefits — that in some cases, collection can be
> faster.  That’s good.  But one of the big costs here is that an interfaces
> that was simple is now considerably less so.  That’s a cost; not convinced
> the two are in balance.
>
>
> On Mar 4, 2019, at 12:35 AM, August Nagro  wrote:
>
> Hi everyone,
>
> My implementation is at
> https://github.com/AugustNagro/presize-collectors-bench and I have a
> webrev here: http://august.nagro.us/presized-collectors/webrev/.
>
> The changes that I made were:
>
>- Add `default IntFunction sizedSupplier()` to Collector
>- Add 2 new Collector.of helper methods taking a sized supplier
>- Update the applicable collectors in Collectors to provide a
>sizedSupplier
>- Have ReferencePipeline & ReduceOps call sizedSupplier when
>appropriate
>
> Some notes/questions I have:
>
>- I considered adding Collector.Characteristics.PRESIZEABLE, but
>figured it was not needed given that sizedSupplier should always delegate
>to supplier.
>- Collector.sizedSupplier could be a LongFunction instead of
>IntFunction (since Sink.begin provides long sizes), but every Collection
>type I've looked at uses int for initialCapacity, so I think IntFunction
>makes more sense.
>- StringJoiner should be updated to take an initalCapacity (should I
>commit this change in the webrev, or leave for another enhancement?)
>- What tests should I add for this enhancement?
>
> There are some benchmarks in the github repo. I was initially surprised
> when I saw that my bare-bones parallel stream did not have a throughput
> improvement like the serial one, but after doing some debugging I think it
> is from Collector.combiner dominating the runtime.
>
> Regards,
>
> August
>
> On Thu, Feb 28, 2019 at 12:56 AM Tagir Valeev  wrote:
>
>> Hello!
>>
>> I wouldn't use presized HashSet, because you never know how many
>> duplicates are expected. What if the input stream has a million of
>> elements, but only two of them are distinct? Do you really want to allocate
>> a hash table for million elements in advance?
>>
>> For toMap() without custom supplier and merger preallocation sounds
>> reasonable as in this case duplicating key is an exceptional case, so we
>> may expect a specific number of elements.
>>
>> чт, 28 февр. 2019 г., 11:38 August Nagro augustna...@gmail.com:
>>
>>> Tagir,
>>>
>>> Great to see the validating benchmarks.
>>>
>>> > I think it's the best solution given the fact that very few collectors may
>>> benefit from the exact known size, and this benefit usually disappears
>>> when collectors are composed (e.g. using groupingBy: downstream
>>> toList() would not win anything if it provides a sizedSupplier).
>>>
>>> I would like to see your benchmarks for that statement too. After all,
>>> Hashmap, HashSet, etc all have presizing constructors, aren't those there
>>> for a reason?
>>>
>>> So I am still convinced that presizing is important for other collector
>>> types, including custom collectors. Although I'm open to having my mind
>>> changed.
>>>
>>>  I'm happy to contribute an implementation as well, I was hoping that
>>> this this (smallish) patch would help me learn the OpenJdk process and make
>>> future contributions easier!
>>>
>>> - August
>>>
>>> On Wed, Feb 27, 2019 at 1:06 AM Tagir Valeev  wrote:
>>>
 Hello!

 > A less intrusive API direction might be a version of Collector whose
 > supplier function took a size-estimate argument; this might even help
 in
 > parallel since it allows for intermediate results to start with a
 better
 > initial size guess.  (And this could be implemented as a default that
 > delegates to the existing supplier.)  Still, not really sure this
 > carries its weight.

 It's interesting that such optimization is possible without API
 change, using the "hidden knowledge".
 While public API stays the same, the collect method implementation may
 check if custom
 non-public Collector implementation is supplied, and in this case may
 use the sized supplier.
 I think it's the best solution given the fact that very few collectors
 may benefit from the exact
 known size, and this benefit usually disappears when 

Re: RFR(S) : 8219139 : move hotspot tests from test/jdk/vm

2019-03-04 Thread mikhailo . seledtsov

Hi Igor,

  Sorry it took a while to get back to you on this one. See my comment 
below



On 2/22/19 10:35 AM, mikhailo.seledt...@oracle.com wrote:
Overall the change looks good; thank you Igor for doing this. I have 
couple of comments:


  - I am in favor of the approach where we move tests first under 
corresponding sub-component directories in
    test/hotspot sub-tree, and then figure out whether to keep them. 
Even though in general I am in favor
    of removing tests that are obsolete or of questionable value, this 
requires time, consideration and discussions.
    Hence, I recommend filing an RFE for that, which can be addressed 
after the migration.


  - Runtime normally avoids tests directly in test/hotspot/jtreg/runtime
    The tests should go into underlying sub-directories which best 
match functional area or topic of that test.
    In most cases you did it in your change, but there are several 
tests that your change places directly under

 test/hotspot/jtreg/runtime/:

 ExplicitArithmeticCheck.java
 MonitorCacheMaybeExpand_DeadLock.java
 ReflectStackOverflow.java
 ShiftTest.java - David commented this can go under compiler (a 
jit test)

 WideStrictInline.java
I have looked at the tests in more detail, and here are my 
recommendation of placements:

    ExplicitArithmeticCheck
    This test checks that ArithmeticException is thrown when 
appropriate

    I would recommend placing it under runtime/ErrorHandling
    MonitorCacheMaybeExpand_DeadLock
    Existing folder: runtime/Thread (it does have a locking test)
    Or, alternatively, create a new folder: 'locking' or 'monitors'
    ReflectStackOverflow
    Uses recursive reflection attempting to provoke stack overflow
    Can go under: runtime/reflect
    WideStrictInline:
    checks for correct FP inlining by the interpreter
    I could not find existing sections; perhaps create 'interpreter'
    folder under 'runtime'

Thank you,
Misha


 Since we plan (as discussed) to follow up this work with an RFE 
to review and consider removal of old and
 not-that-useful tests, you could place them under 'misc' for now. 
Alternatively, find the best match

 or create new sub-directories under runtime/ if necessary.


Thank you,
Misha


On 2/21/19 11:53 AM, Igor Ignatyev wrote:


On Feb 21, 2019, at 12:11 AM, David Holmes  
wrote:


Hi Igor,

On 21/02/2019 3:19 pm, Igor Ignatyev wrote:

http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html

40 lines changed: 17 ins; 2 del; 21 mod;

Hi all,
could you please review this small patch which moves tests from 
test/jdk/vm?
I find some of these tests - the runtime ones at least - of 
extremely dubious value. They either cover basic functionality that 
is already well covered, or are regression tests for bugs in code 
that hasn't existed for many many years!
as I wrote in another related email: "one of the reason I'm proposing 
this move is exactly questionable value of these tests, I want to 
believe that having these tests in hotspot/ test directories will 
bring more attention to them from corresponding component teams and 
hence they will be removed/reworked/re-whatever faster and better. 
and I also believe that one of the reason we got duplications exactly 
because these tests were located in jdk test suite."



BTW:

test/hotspot/jtreg/runtime/ShiftTest.java

is actually a jit test according to the test comment.

sure, I will move it to hotspot/compiler.
there are 16 tests in test/jdk/vm directory. all but 
JniInvocationTest are hotspot tests, so they are moved to 
test/hotspot test suite; JniInvocationTest is a launcher test
No its a JNI invocation API test - nothing to do with our launcher. 
It belongs in runtime/jni. But we already have tests in runtime that 
use the JNI invocation API so this test adds no new coverage.
this is actually was my first reaction, and I even have the webrev 
which moves it to runtime/jni, but then I looked at the associated 
bug and it is filed against tools/launcher. and I even got a false 
(as I know by now) memory that I saw JLI_ method being called from 
the test. there is actually another test 
(dk/tools/launcher/exeJliLaunchTest.c) associated w/ this bug which 
calls JLI_Launch. anyhow, I'll move this test to hotspot/runtime/jni.


I really think the value of these tests needs to be examined before 
they are brought over.
I'd prefer to have follow-up RFEs/tasks, b/c the longer we keep 
jdk/vm directory the more tests can end up there and the more rotten 
these tests become.


Thanks,
-- Igor

Thanks,
David
-

and hence it's moved to test/jdk/tools/launcher. as 
hotspot/compiler and hotspot/gc tests use packages, the tests moved 
there have been updated to have a package statement.
webrev: 
http://cr.openjdk.java.net/~iignatyev//8219139/webrev.00/index.html

JBS: https://bugs.openjdk.java.net/browse/JDK-8219139
testing: tier[1-2] (in progress), all the touched tests locally

Re: Proposal to enhance Stream.collect

2019-03-04 Thread Brian Goetz
So, unfortunately I think this particular API evolution idiom is kind of 
questionable.  There is a new method, sizedSupplier, with a default that 
delegates to the unsized supplier.  But, that leaves implementors in a position 
where they don’t really know which one to implement, and the use of 
characteristics to hint at which one to call is kind of convoluted.  

You’re focused on the benefits — that in some cases, collection can be faster.  
That’s good.  But one of the big costs here is that an interfaces that was 
simple is now considerably less so.  That’s a cost; not convinced the two are 
in balance.

> On Mar 4, 2019, at 12:35 AM, August Nagro  wrote:
> 
> Hi everyone,
> 
> My implementation is at 
> https://github.com/AugustNagro/presize-collectors-bench 
>  and I have a webrev 
> here: http://august.nagro.us/presized-collectors/webrev/ 
> .
> 
> The changes that I made were:
> Add `default IntFunction sizedSupplier()` to Collector
> Add 2 new Collector.of helper methods taking a sized supplier
> Update the applicable collectors in Collectors to provide a sizedSupplier
> Have ReferencePipeline & ReduceOps call sizedSupplier when appropriate
> Some notes/questions I have:
> I considered adding Collector.Characteristics.PRESIZEABLE, but figured it was 
> not needed given that sizedSupplier should always delegate to supplier.
> Collector.sizedSupplier could be a LongFunction instead of IntFunction (since 
> Sink.begin provides long sizes), but every Collection type I've looked at 
> uses int for initialCapacity, so I think IntFunction makes more sense.
> StringJoiner should be updated to take an initalCapacity (should I commit 
> this change in the webrev, or leave for another enhancement?)
> What tests should I add for this enhancement? 
> There are some benchmarks in the github repo. I was initially surprised when 
> I saw that my bare-bones parallel stream did not have a throughput 
> improvement like the serial one, but after doing some debugging I think it is 
> from Collector.combiner dominating the runtime.
> 
> Regards,
> 
> August
> 
> On Thu, Feb 28, 2019 at 12:56 AM Tagir Valeev  > wrote:
> Hello! 
> 
> I wouldn't use presized HashSet, because you never know how many duplicates 
> are expected. What if the input stream has a million of elements, but only 
> two of them are distinct? Do you really want to allocate a hash table for 
> million elements in advance?
> 
> For toMap() without custom supplier and merger preallocation sounds 
> reasonable as in this case duplicating key is an exceptional case, so we may 
> expect a specific number of elements.
> 
> чт, 28 февр. 2019 г., 11:38 August Nagro augustna...@gmail.com 
> :
> Tagir,
> 
> Great to see the validating benchmarks.
> 
> > I think it's the best solution given the fact that very few collectors may 
> > benefit from the exact known size, and this benefit usually disappears when 
> > collectors are composed (e.g. using groupingBy: downstream toList() would 
> > not win anything if it provides a sizedSupplier).
> 
> I would like to see your benchmarks for that statement too. After all, 
> Hashmap, HashSet, etc all have presizing constructors, aren't those there for 
> a reason?
> 
> So I am still convinced that presizing is important for other collector 
> types, including custom collectors. Although I'm open to having my mind 
> changed.  
> 
>  I'm happy to contribute an implementation as well, I was hoping that this 
> this (smallish) patch would help me learn the OpenJdk process and make future 
> contributions easier!
> 
> - August
> 
> On Wed, Feb 27, 2019 at 1:06 AM Tagir Valeev  > wrote:
> Hello!
> 
> > A less intrusive API direction might be a version of Collector whose
> > supplier function took a size-estimate argument; this might even help in
> > parallel since it allows for intermediate results to start with a better
> > initial size guess.  (And this could be implemented as a default that
> > delegates to the existing supplier.)  Still, not really sure this
> > carries its weight.
> 
> It's interesting that such optimization is possible without API
> change, using the "hidden knowledge".
> While public API stays the same, the collect method implementation may
> check if custom
> non-public Collector implementation is supplied, and in this case may
> use the sized supplier.
> I think it's the best solution given the fact that very few collectors
> may benefit from the exact
> known size, and this benefit usually disappears when collectors are
> composed (e.g. using groupingBy:
> downstream toList() would not win anything if it provides a sizedSupplier).
> 
> I created a quick patch and launched a quick benchmark like this:
> return new SplittableRandom(0).ints(length, 0,
> 100).boxed().collect(Collectors.toList());
> For length = 

[8u-dev] Request for Approval: Backport of JDK-8217609 : New era placeholder not recognized by java.text.SimpleDateFormat

2019-03-04 Thread Deepak Kejriwal
Hi All,

 

Please approve the backport of following bugs to 8u-dev.

 

JDK- 8217609 : New era placeholder not recognized by java.text.SimpleDateFormat

 

JDK Review Thread: 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-March/058786.html

 

All the related testing is done and is a pass.

 

Regards,

Deepak

 


Re: RFR: JDK8U Backport of 8217609: New era placeholder not recognized by java.text.SimpleDateFormat

2019-03-04 Thread Naoto Sato

Looks good.

Naoto

On 3/4/19 12:56 AM, Deepak Kejriwal wrote:

Hi Naoto,

Thanks for review. I as update the webrev as per the review comments. Please 
find below updated version of webrev:
http://cr.openjdk.java.net/~pkoppula/dkejriwal/8217609/webrev.01/

Since long and Japanese Locale issue is specific to 8u and not related to new 
era, I have raised a new issue JDK-8220020 for it.

Regards,
Deepak

-Original Message-
From: Naoto Sato
Sent: Friday, March 1, 2019 11:07 PM
To: Deepak Kejriwal ; core-libs-dev 
; jdk8u-...@openjdk.java.net
Subject: Re: RFR: JDK8U Backport of 8217609: New era placeholder not recognized 
by java.text.SimpleDateFormat

Hi Deepak,

A few comments on the JapaneseEraNameTest:

- Please indent code blocks accordingly, i.e., names object declaration (line 
47-51), for loop in the main() (line 56,59,60).

- If you are commenting out the case for LONG, please address the reason with 
the comment (the following explanation will do)

- If the above issue is specific to 8u, and not related to the new era, file a 
separate issue for 8u.

Naoto

On 3/1/19 2:44 AM, Deepak Kejriwal wrote:

Hi All,

   


Please review the back port of fix for JDK-8217609 to 8u-dev:-

   


JBS report: HYPERLINK
"https://bugs.openjdk.java.net/browse/JDK-%208217609"https://bugs.open
jdk.java.net/browse/JDK- 8217609

Webrev:
http://cr.openjdk.java.net/~pkoppula/dkejriwal/8217609/webrev.00/

Master bug change set:
http://hg.openjdk.java.net/jdk/jdk/rev/8ea340a71f17

Summary:
Fix is backported from JDK 13 to JDK 8u and is not a clean backport.
Reason for that is because JDK uses CLDR version 21.01 and JDK 13 uses
version 33. Below are the differences between 8u and 13:-

. The "jp.xml" version of JDK 8u does not contains "" node (). 
Therefore "N" which is part of fix is not added to file.

. I've commented out one of the test data { LONG, JAPAN, "\u5143\u53f7" } in 
"JapaneseEraNameTest.java"  for JDK 8u.  It checks era name returned by method 
"java.util.Calendar.getDisplayName(int field, int style, Locale locale)"

o   The  test fails for this particular data on prior 8u releases across all 
existing eras(HEISI,SHOWA,..).

o   The cause of issue is that CLDR java resource file (FormatData_ja.java) generated by "CLDR Converter" does not 
contain required resource key "japanese.long.Eras".  The "CLDR Converter" need to be fixed to address this 
issue. Since this issue is an existing issue data point { LONG, JAPAN, "\u5143\u53f7" } is removed from 
"JapaneseEraNameTest.java" for 8u.

Regards,

Deepak

   



Re: Proposal: JDK-8148917 Enhanced-For Statement Should Allow Streams

2019-03-04 Thread Stephen Colebourne
On Fri, 1 Mar 2019 at 02:46, Stuart Marks  wrote:
> Please review and comment on this proposal to allow Stream instances to be 
> used
> in enhanced-for ("for-each") loops.

This all seems reasonable enough though of course not ideal.

My slight concern is that the terminal operation is hidden and not
immediately visible, which could be surprising. I do note that streams
throw a clear exception if a terminal operation is called a second
time which mitigates this to some degree.

The IterableOnce class-level Javadoc contradicts the method-level
Javadoc. The class-level say "It is recommended that" whereas the
method-level Javadoc mandates.

Stephen



> Abstract
>
> Occasionally it's useful to iterate a Stream using a conventional loop. 
> However,
> the Stream interface doesn't implement Iterable, and therefore streams cannot 
> be
> used with the enhanced-for statement. This is a proposal to remedy that
> situation by introducing a new interface IterableOnce that is a subtype of
> Iterable, and then retrofitting the Stream interface to implement it. Other 
> JDK
> classes will also be retrofitted to implement IterableOnce.
>
> Full Proposal:
>
>  http://cr.openjdk.java.net/~smarks/reviews/8148917/IterableOnce0.html
>
> Bug report:
>
>  https://bugs.openjdk.java.net/browse/JDK-8148917
>
> Webrev:
>
>  http://cr.openjdk.java.net/~smarks/reviews/8148917/webrev.0/
>
> Note, this changeset isn't ready to push yet. In particular, it has no tests
> yet. However, the implementation is so simple that I figured I should include
> it. Comments on the specification wording are also welcome.
>
> Thanks,
>
> s'marks


Re: JDK-8217735: Q: Should jint be jboolean ?

2019-03-04 Thread Andrew Leonard
Thanks David, that's a good thought, so yes jboolean looks right form 
that.
Cheers
Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 




From:   David Holmes 
To: Andrew Leonard , core-libs-dev 

Date:   04/03/2019 12:52
Subject:Re: JDK-8217735: Q: Should jint be jboolean ?



On 4/03/2019 9:30 pm, Andrew Leonard wrote:
> Hi,
> 
> This bug raised a missmatch between the Java and JNI definition for a
> native method, where Java specifies "boolean" and the JNI jint. Which is
> right, should they match?
> 
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8217735=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=NaV8Iy8Ld-vjpXZFDdTbgGlRTghGHnwM75wUPd5_NUQ=KoeDVOc0vzvP0wizDDykj53gxMU5Zcop2nu4FsOonUQ=H_SBb8AJvNppMzFArFxWqfF-ss0W3le6MVndHorE7qU=


Old javah generates jboolean

  > cat Native.java
public class Native {
   public native boolean isTrue(boolean b);
}

  > cat Native.h
/* DO NOT EDIT THIS FILE - it is machine generated */
#include 
/* Header for class Native */

#ifndef _Included_Native
#define _Included_Native
#ifdef __cplusplus
extern "C" {
#endif
/*
  * Class: Native
  * Method:isTrue
  * Signature: (Z)Z
  */
JNIEXPORT jboolean JNICALL Java_Native_isTrue
   (JNIEnv *, jobject, jboolean);

#ifdef __cplusplus
}
#endif
#endif

David
-

> One part of the JVM spec that confuses me a bit is:
> 2.3.4 The boolean Type
> Although the Java Virtual Machine defines a boolean type,
> it only provides very limited support for it.
> There are no Java Virtual Machine instructions solely dedicated
> to operations on boolean values. Instead, expressions in the Java
> programming language that operate on boolean values are
> compiled to use values of the Java Virtual Machine int data type.
> 
> Thanks
> Andrew
> 
> Andrew Leonard
> Java Runtimes Development
> IBM Hursley
> IBM United Kingdom Ltd
> Phone internal: 245913, external: 01962 815913
> internet email: andrew_m_leon...@uk.ibm.com
> 
> Unless stated otherwise above:
> IBM United Kingdom Limited - Registered in England and Wales with number
> 741598.
> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 
3AU
> 




Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU


Re: JDK-8217735: Q: Should jint be jboolean ?

2019-03-04 Thread David Holmes

On 4/03/2019 9:30 pm, Andrew Leonard wrote:

Hi,

This bug raised a missmatch between the Java and JNI definition for a
native method, where Java specifies "boolean" and the JNI jint. Which is
right, should they match?
https://bugs.openjdk.java.net/browse/JDK-8217735


Old javah generates jboolean

 > cat Native.java
public class Native {
  public native boolean isTrue(boolean b);
}

 > cat Native.h
/* DO NOT EDIT THIS FILE - it is machine generated */
#include 
/* Header for class Native */

#ifndef _Included_Native
#define _Included_Native
#ifdef __cplusplus
extern "C" {
#endif
/*
 * Class: Native
 * Method:isTrue
 * Signature: (Z)Z
 */
JNIEXPORT jboolean JNICALL Java_Native_isTrue
  (JNIEnv *, jobject, jboolean);

#ifdef __cplusplus
}
#endif
#endif

David
-


One part of the JVM spec that confuses me a bit is:
2.3.4 The boolean Type
Although the Java Virtual Machine defines a boolean type,
it only provides very limited support for it.
There are no Java Virtual Machine instructions solely dedicated
to operations on boolean values. Instead, expressions in the Java
programming language that operate on boolean values are
compiled to use values of the Java Virtual Machine int data type.

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number
741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU



Re: JDK-8217735: Q: Should jint be jboolean ?

2019-03-04 Thread Alan Bateman

On 04/03/2019 11:30, Andrew Leonard wrote:

Hi,

This bug raised a missmatch between the Java and JNI definition for a
native method, where Java specifies "boolean" and the JNI jint. Which is
right, should they match?
https://bugs.openjdk.java.net/browse/JDK-8217735

It's probably best to bring this to awt-dev to get to the right people 
that maintain this area.


-Alan


JDK-8217735: Q: Should jint be jboolean ?

2019-03-04 Thread Andrew Leonard
Hi,

This bug raised a missmatch between the Java and JNI definition for a 
native method, where Java specifies "boolean" and the JNI jint. Which is 
right, should they match?
https://bugs.openjdk.java.net/browse/JDK-8217735
One part of the JVM spec that confuses me a bit is:
2.3.4 The boolean Type
Although the Java Virtual Machine defines a boolean type, 
it only provides very limited support for it. 
There are no Java Virtual Machine instructions solely dedicated
to operations on boolean values. Instead, expressions in the Java 
programming language that operate on boolean values are 
compiled to use values of the Java Virtual Machine int data type.

Thanks
Andrew

Andrew Leonard
Java Runtimes Development
IBM Hursley
IBM United Kingdom Ltd
Phone internal: 245913, external: 01962 815913
internet email: andrew_m_leon...@uk.ibm.com 

Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU


RE: RFR: JDK8U Backport of 8217609: New era placeholder not recognized by java.text.SimpleDateFormat

2019-03-04 Thread Deepak Kejriwal
Hi Andrew,

Thanks for review. As of now we don't which bug need to be backported to 8u for 
fixing CLDR convertor issue. However, I have raised a separate bug for same:-
https://bugs.openjdk.java.net/browse/JDK-8220020

Regards,
Deepak 

-Original Message-
From: Andrew Hughes  
Sent: Friday, March 1, 2019 9:09 PM
To: Deepak Kejriwal 
Cc: core-libs-dev ; jdk8u-dev 

Subject: Re: RFR: JDK8U Backport of 8217609: New era placeholder not recognized 
by java.text.SimpleDateFormat

On Fri, 1 Mar 2019 at 10:47, Deepak Kejriwal  wrote:
>
> Hi All,
>
>
>
> Please review the back port of fix for JDK-8217609 to 8u-dev:-
>
>
>
> JBS report: HYPERLINK 
> "https://bugs.openjdk.java.net/browse/JDK-%208217609%22https://bugs.op
> enjdk.java.net/browse/JDK- 8217609
>
> Webrev: 
> http://cr.openjdk.java.net/~pkoppula/dkejriwal/8217609/webrev.00/
>
> Master bug change set:  
> http://hg.openjdk.java.net/jdk/jdk/rev/8ea340a71f17
>
> Summary:
> Fix is backported from JDK 13 to JDK 8u and is not a clean backport. 
> Reason for that is because JDK uses CLDR version 21.01 and JDK 13 uses 
> version 33. Below are the differences between 8u and 13:-
>
> . The "jp.xml" version of JDK 8u does not contains "" node 
> (). Therefore "N" which is 
> part of fix is not added to file.
>
> . I've commented out one of the test data { LONG, JAPAN, 
> "\u5143\u53f7" } in "JapaneseEraNameTest.java"  for JDK 8u.  It checks era 
> name returned by method "java.util.Calendar.getDisplayName(int field, int 
> style, Locale locale)"
>
> o   The  test fails for this particular data on prior 8u releases across all 
> existing eras(HEISI,SHOWA,..).
>
> o   The cause of issue is that CLDR java resource file (FormatData_ja.java) 
> generated by "CLDR Converter" does not contain required resource key 
> "japanese.long.Eras".  The "CLDR Converter" need to be fixed to address this 
> issue. Since this issue is an existing issue data point { LONG, JAPAN, 
> "\u5143\u53f7" } is removed from "JapaneseEraNameTest.java" for 8u.
>
> Regards,
>
> Deepak
>
>

Patch looks good to me.

Is there a bug/patch to backport to fix the CLDR converter in OpenJDK 8, so 
japanese.long.Eras are included?

JDK-8217609 should have the label 'jdk8u-fix-request' added so it can be 
approved for 8u.

Thanks,
--
Andrew :)

Senior Free Java Software Engineer
Red Hat, Inc. 
(https://urldefense.proofpoint.com/v2/url?u=http-3A__www.redhat.com=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=OIRLm_rdyVUfNJH3SLA-tRzXEZ3NIKC14aO9CokPvgI=j3CCAe0gKdZ1tUX3wqQmQH0-QEcf9c-SP_A6NIFBobU=taCA3iR6U95qSdORcSSBkhnzDPXZDhrUKtyZjecFOGw=)

Web Site: 
https://urldefense.proofpoint.com/v2/url?u=http-3A__fuseyism.com=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=OIRLm_rdyVUfNJH3SLA-tRzXEZ3NIKC14aO9CokPvgI=j3CCAe0gKdZ1tUX3wqQmQH0-QEcf9c-SP_A6NIFBobU=qf3t66gFRZcTpETTq7B7ur6kxz7LEX2z65dog1MIlKg=
Twitter: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__twitter.com_gnu-5Fandrew-5Fjava=DwIFaQ=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE=OIRLm_rdyVUfNJH3SLA-tRzXEZ3NIKC14aO9CokPvgI=j3CCAe0gKdZ1tUX3wqQmQH0-QEcf9c-SP_A6NIFBobU=Kjo5wWD8vSXUKQhnMlZ9dNyEQWdR1QuHvqPAwz5hJ7c=
PGP Key: ed25519/0xCFDA0F9B35964222 (hkp://keys.gnupg.net) Fingerprint = 5132 
579D D154 0ED2 3E04  C5A0 CFDA 0F9B 3596 4222


RE: RFR: JDK8U Backport of 8217609: New era placeholder not recognized by java.text.SimpleDateFormat

2019-03-04 Thread Deepak Kejriwal
Hi Naoto,

Thanks for review. I as update the webrev as per the review comments. Please 
find below updated version of webrev:
http://cr.openjdk.java.net/~pkoppula/dkejriwal/8217609/webrev.01/

Since long and Japanese Locale issue is specific to 8u and not related to new 
era, I have raised a new issue JDK-8220020 for it.

Regards,
Deepak

-Original Message-
From: Naoto Sato 
Sent: Friday, March 1, 2019 11:07 PM
To: Deepak Kejriwal ; core-libs-dev 
; jdk8u-...@openjdk.java.net
Subject: Re: RFR: JDK8U Backport of 8217609: New era placeholder not recognized 
by java.text.SimpleDateFormat

Hi Deepak,

A few comments on the JapaneseEraNameTest:

- Please indent code blocks accordingly, i.e., names object declaration (line 
47-51), for loop in the main() (line 56,59,60).

- If you are commenting out the case for LONG, please address the reason with 
the comment (the following explanation will do)

- If the above issue is specific to 8u, and not related to the new era, file a 
separate issue for 8u.

Naoto

On 3/1/19 2:44 AM, Deepak Kejriwal wrote:
> Hi All,
> 
>   
> 
> Please review the back port of fix for JDK-8217609 to 8u-dev:-
> 
>   
> 
> JBS report: HYPERLINK 
> "https://bugs.openjdk.java.net/browse/JDK-%208217609"https://bugs.open
> jdk.java.net/browse/JDK- 8217609
> 
> Webrev: 
> http://cr.openjdk.java.net/~pkoppula/dkejriwal/8217609/webrev.00/
> 
> Master bug change set:  
> http://hg.openjdk.java.net/jdk/jdk/rev/8ea340a71f17
> 
> Summary:
> Fix is backported from JDK 13 to JDK 8u and is not a clean backport. 
> Reason for that is because JDK uses CLDR version 21.01 and JDK 13 uses 
> version 33. Below are the differences between 8u and 13:-
> 
> . The "jp.xml" version of JDK 8u does not contains "" node 
> (). Therefore "N" which is 
> part of fix is not added to file.
> 
> . I've commented out one of the test data { LONG, JAPAN, 
> "\u5143\u53f7" } in "JapaneseEraNameTest.java"  for JDK 8u.  It checks era 
> name returned by method "java.util.Calendar.getDisplayName(int field, int 
> style, Locale locale)"
> 
> o   The  test fails for this particular data on prior 8u releases across all 
> existing eras(HEISI,SHOWA,..).
> 
> o   The cause of issue is that CLDR java resource file (FormatData_ja.java) 
> generated by "CLDR Converter" does not contain required resource key 
> "japanese.long.Eras".  The "CLDR Converter" need to be fixed to address this 
> issue. Since this issue is an existing issue data point { LONG, JAPAN, 
> "\u5143\u53f7" } is removed from "JapaneseEraNameTest.java" for 8u.
> 
> Regards,
> 
> Deepak
> 
>   
>