Re: 8202469 / 8202473: Correct type annotation resolution for class type variables

[Rafael Winterhalter]

I finally found the time to look at this again, sorry for the delay.

Thank you for your comments. I had the chance to better look into the
problem and simplify the code a bit more and also reduced the scope of the
fix to a single problem. I also added test cases that should be exhaustive
for all possible scenarios and adjusted the code comment.

I uploaded the adjusted patch as a webrev:
http://cr.openjdk.java.net/~winterhalter/8202469/webrev.01/

Thanks, Rafael

Am So., 16. Feb. 2020 um 10:51 Uhr schrieb Joel Borggrén-Franck <
joel.borggren.fra...@gmail.com>:

> Hi Rafael,
>
> Thanks for reaching out and reminding me of this!
>
> I managed to find some time to look into 8202469 during the weekend. By
> swapping place of  the TVars in the test I managed to isolate this without
> depending on 8202473, take a look at my hacky copy-paste update to your
> test at http://cr.openjdk.java.net/~jfranck/8202469/ .
>
> The comment on lines 269-276 needs to be updated. Perhaps include a table
> with the start index depending of the type? Also referencing JVMLS 4.4 for
> the structure of the bound might be instructive for future readers.
>
> My current understanding is that there are two problems with the code on
> (the original) lines 277-287:
> 1) Type Variables should have index 0 while getting index 1 due to lines
> 279-280.
> 2) Bounds that are instances of ParameterizedType always gets index 1 but
> if the first bound represents a Class type the index should be 0.
>
> Does this make sense?
>
> Can you make the if-switches positive? I find my original code with the
> negative tests hard to read and the update doesn't help.
>
> Also can you expand the test to cover the different kinds of bounds
> mentioned in 4.4 and also test Type Variables on methods, I suspect javac
> treats method (and constructor) tvars similarly but there have been bugs ...
>
> TL;DR please update the webrev with:
>
> - Split out test and fix for 8202469
> - More test coverage of different kinds of bounds
> - Test for method tvars
> - See if you can rework the logic to have (mostly) positive tests in if
> switch
>
> Thanks again for looking into this, I'll start looking into 8202473, I
> think the fix for that one opens up a bigger rework of the code which is
> why I want to separate the two.
>
> cheers
> /Joel
>
> On Sun, Aug 4, 2019 at 10:12 PM Rafael Winterhalter 
> wrote:
>
>> Hi,
>>
>> appologies for the delay, I only managed to get my patched up to webrev
>> today (http://cr.openjdk.java.net/~winterhalter/). It's three patches in
>> total now, for the third one I could not add a test case, it would require
>> to compile an annotation property against an enumeration type and load it
>> against another class where the enumeration was turned into an annotation.
>> I struggled a bit with jtreg to make it work but I cannot see myself
>> complete this in a meaningful amount of time. However, I hope that the
>> patch and the error it solves are straightforward to see.
>>
>> I only submitted a patch for 8202469. 8202473 is solved by it. It's two
>> bugs but they are a symptom of the same oversight.
>>
>> I hope this helps you to review it but let me know if you have any
>> questions or if I should further adjust my patch.
>>
>> Best regards, Rafael
>>
>> Am Fr., 2. Aug. 2019 um 12:18 Uhr schrieb Rafael Winterhalter <
>> rafael@gmail.com>:
>>
>>> Thanks Daniel,
>>> I did some work on Glassfish a bunch of years ago, I had no idea.
>>> I will try to split up the changes (I fixed another bug in annotation
>>> processing yesterday) and upload everything there.
>>> Best regards, Rafael
>>>
>>> Am Fr., 2. Aug. 2019 um 11:59 Uhr schrieb Daniel Fuchs <
>>> daniel.fu...@oracle.com>:
>>>
 Hi Rafael,

 On 02/08/2019 09:00, Joel Borggrén-Franck wrote:
 > I can host webrevs for you on cr to make it easier for other
 reviewers, if
 > you also send me the patches or webrefs off-list to get around the
 > attachment stripping I can upload them to cr.

 I see that you are a JDK project author, so you already own a page
 on cr (http://cr.openjdk.java.net/~winterhalter/) where you can
 upload webrevs (e.g. using `scp` as in:
 $ scp -r webrev-N.01 winterhal...@cr.openjdk.java.net: )

 best regards and welcome!

 -- daniel

>>>

RFR: 8240629: argfiles parsing broken for argfiles with comment cross 4096 bytes chunk

[Henry Jen]

Hi,

Please review the webrev[1] fix JDK-8240629 reported earlier by Robert.

http://cr.openjdk.java.net/~henryjen/jdk/8240629.0/webrev/

Cheers,
Henry

Re: [15] 8235792: LineNumberReader.getLineNumber() behavior is inconsistent with respect to EOF

[Brian Burkhalter]

Thanks, Roger. I’ll let it hang until next week and then file a CSR.

Brian

> On Mar 6, 2020, at 10:49 AM, Roger Riggs  wrote:
> 
> :) Still looks fine.
> 
> On 3/5/20 4:53 PM, Brian Burkhalter wrote:
>> Might anyone else have any comments on this thread the original post in 
>> which was [1].
>> 
>> Thanks,
>> 
>> Brian
>> 
>> [1] 
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.html
>>  
>> 

Re: [15] 8235792: LineNumberReader.getLineNumber() behavior is inconsistent with respect to EOF

[Roger Riggs]

:) Still looks fine.

On 3/5/20 4:53 PM, Brian Burkhalter wrote:

Might anyone else have any comments on this thread the original post in which 
was [1].

Thanks,

Brian

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2020-January/064417.html


On Jan 31, 2020, at 12:21 PM, Brian Burkhalter  
wrote:


On Jan 31, 2020, at 12:20 PM, Alan Bateman mailto:alan.bate...@oracle.com>> wrote:

On 31/01/2020 20:10, Brian Burkhalter wrote:

:
OK. I’ll change /TERM/EOL/ but not post another webrev unless there are further 
comments. I will wait to check it in until Monday in case someone has comments.


I think it would be good to more eyes on this as LNR has a history of resisting 
change.

I’ll hold off on it then for a while.


I think you'll need a CSR too this re-specifies when the conditions when the 
line number is incremented.

Yes, I agree.

Re: RFR: 8216407: java.util.UUID.fromString accepts input that does not match expected format

[Roger Riggs]

Hi  Chihiro, et.al.,

Thanks for taking a look at this issue,  however...

There has been a long history of concerns[1] about breaking existing 
applications
that depend on the loose parsing of UUIDs.  Throwing an exception where 
it did not

previously is an incompatible change.

The crucial concern about performance parsing conforming strings has 
been addressed by:


8196334 Optimize UUID#fromString 



I propose to close these as WILL-NOT-FIX: and hope that the next several 
times it gets reported

they will be closed as duplicates.

8216407    
java.util.UUID.fromString accepts input that does not match expected format

8165199 
UUID.fromString 
accepts wrong placements of the dashes


Any other suggestions welcome.

Thanks, Roger

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-December/057470.html



On 3/2/20 10:39 AM, Chihiro Ito wrote:

Hi,

I tried to correct this problem.

Could you review this fix, please?

According to the RFC 4122, UUID has a fixed format. I tried to raise an
exception if a string was specified that is not suitable for this
format. Also, is there anything else I should be aware of with this bug?

Webrev : http://cr.openjdk.java.net/~cito/JDK-8216407/webrev.00/
JBS : https://bugs.openjdk.java.net/browse/JDK-8216407

Regards,
Chihiro

Re: PING: RFR: 8216407: java.util.UUID.fromString accepts input that does not match expected format

[Joe Darcy]

Hello Chihiro,

Note that as the proposed change affects behavioral compatibility, it 
will need to have CSR review 
(https://wiki.openjdk.java.net/display/csr/Main) in addition to code review.


Reviewers might find it helpful to have a written categorization of the 
strings this change rejects that are now accepted.


Thanks,

-Joe

On 3/6/2020 7:25 AM, Chihiro Ito wrote:

Hi,

I tried to correct this problem.

Could you review this fix, please?

According to the RFC 4122, UUID has a fixed format. I tried to raise
an exception if a string was specified that is not suitable for this
format. Also, is there anything else I should be aware of with this
bug?

Webrev : http://cr.openjdk.java.net/~cito/JDK-8216407/webrev.00/
JBS : https://bugs.openjdk.java.net/browse/JDK-8216407

Regards,
Chihiro

2020年3月3日(火) 0:39 Chihiro Ito :

Hi,

I tried to correct this problem.

Could you review this fix, please?

According to the RFC 4122, UUID has a fixed format. I tried to raise an 
exception if a string was specified that is not suitable for this format. Also, 
is there anything else I should be aware of with this bug?

Webrev : http://cr.openjdk.java.net/~cito/JDK-8216407/webrev.00/
JBS : https://bugs.openjdk.java.net/browse/JDK-8216407

Regards,
Chihiro

Re: RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

[naoto . sato]

Looks good. Thanks for the update.

Naoto

On 3/6/20 8:17 AM, Roger Riggs wrote:

Hi Naoto,

Updated webrev:
http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html

I don't think getting handle count of your own process can ever fail.
But in the abundance of caution, added a check and failure.

Copyright updated.

Thanks, Roger


Corrected copyright

On 3/5/20 4:14 PM, naoto.s...@oracle.com wrote:

Hi Roger,

In the test, the error value (-1) from the native code seems silently 
suppressed. Should it be caught/reported in the java side?


nit: copyright year in ProcessImpl.java -> 2020.

Naoto

On 3/5/20 12:51 PM, Roger Riggs wrote:
Please review a change to the Windows ProcessImpl to ensure that the 
handles
created for the input and output streams of a process are closed when 
no longer referenced.


Unlike on Linux, there is no thread monitoring the process that can 
close the streams.
The FileDescriptors are registered with the Cleaner to be closed when 
they are no longer referenced.


A test is added that monitors the count of handles as 50 Processes 
are launched and exit.

The test and change only affect the Windows implementation.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8239893

Thanks, Roger

Re: RFR(S): 8232081: Try to link all classes during dynamic CDS dump

[Calvin Cheung]

Hi Ioi,

On 3/5/20 9:16 PM, Ioi Lam wrote:

Hi Calvin,

Looks good.

Thanks for taking another look.


In JDK-8240244, I removed "ik->loader_type() != 0" because it's 
unclear what it means. I replaced with a new method 
InstanceKlass::is_shared_unregistered_class(). I think you can use 
this in your patch instead of adding loader_type() back.


I'll make the change and do more testing before push.

thanks,

Calvin



No need for new webrev.

Thanks
- Ioi


On 3/5/20 11:48 AM, Calvin Cheung wrote:


On 3/5/20 9:17 AM, Ioi Lam wrote:

Hi Calvin,

Looks good overall. I just have two comment:

I think this is not needed:

 306 void ConstantPool::resolve_class_constants(TRAPS) {
 307   Arguments::assert_is_dumping_archive();

I've reverted this change.


because this function is used to resolve all Strings in the 
statically dumped classes to archive all the Strings. However, the 
archive heap is not supported for the dynamic archive. So I think 
it's better to avoid calling this function from 
LinkSharedClassesClosure for dynamic dump.

Now, the function will not be called with dynamic dump:
1714 if (DumpSharedSpaces) {
1715   // The following function is used to resolve all 
Strings in the statically
1716   // dumped classes to archive all the Strings. The 
archive heap is not supported

1717   // for the dynamic archive.
1718 ik->constants()->resolve_class_constants(THREAD);
1719 }


LinkSharedClassesClosure::_is_static -- maybe we can avoid adding 
this field and just check "if (DumpSharedSpaces)" instead?


This is a good simplification.

btw, you've removed loader_type() from instanceKlass.hpp when you 
pushed the change for JDK-8240244. I've added it back as 
shared_loader_type().


updated webrev:

    http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.04/

thanks,

Calvin



Thanks
- Ioi





On 3/4/20 9:13 PM, Calvin Cheung wrote:

Hi David, Ioi,

Here's an updated webrev which doesn't involve changing 
InstanceKlass::verify_on() and made use of the changes for 
JDK-8240481.


http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.03/

thanks,
Calvin

On 3/1/20 10:14 PM, David Holmes wrote:

Hi Calvin,

On 28/02/2020 4:12 pm, Calvin Cheung wrote:

Hi David,

On 2/27/20 5:40 PM, David Holmes wrote:

Hi Calvin, Ioi,

Looking good - comments below.

A meta-question: normal application classes are rarely loaded 
but not linked so I'm a little surprised this is an issue. What 
is the main source of such classes - generated classes like 
lambda forms? Also why do we need to link them when loading from 
the archive if they were never linked when the application 
actually executed ??


I saw that Ioi already answered the above.

I'll try to answer your questions inline below..


Responses inline below ...



On 28/02/2020 10:48 am, Calvin Cheung wrote:

Hi David, Ioi,

Thanks for your review and suggestions.

On 2/26/20 9:46 PM, Ioi Lam wrote:



On 2/26/20 7:50 PM, David Holmes wrote:

Hi Calvin,

Adding core-libs-dev as you are messing with their code :)

On 27/02/2020 1:19 pm, Calvin Cheung wrote:

JBS: https://bugs.openjdk.java.net/browse/JDK-8232081
webrev: 
http://cr.openjdk.java.net/~ccheung/jdk15/8232081/webrev.00/


The proposed changeset for this RFE adds a 
JVM_LinkClassesForCDS() function to be called from 
java/lang/Shutdown to notify the JVM to link the classes 
loaded by the builtin class loaders. The 


This would be much less disruptive if this was handled purely 
on the VM side once we have started shutdown. No need to make 
any changes to the Java side then, nor jvm.cpp.




Hi David,

To link the classes, we need to be able to run Java code -- 
when linking a class X loaded by the app loader, X will be 
verified. During verification of X, we may need to load 
additional classes from the app loader, which executes Java 
code during its class loading operations.


We also need to handle all the exit conditions. As far as I 
can tell, an app can exit the JVM in two ways:


(1) Explicitly calling System.exit(). This will call 
java.lang.Shutdown.exit() which does this:


http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l163 



    beforeHalt(); // native
    runHooks();
    halt(status);

(2) When all non-daemon threads have died (e.g., falling out 
of the bottom of HelloWorld.main()). There's no explicit call 
to System.exit(), but the JVM will proactively call 
java.lang.Shutdown.shutdown() inside 
JavaThread::invoke_shutdown_hooks()


http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/hotspot/share/runtime/thread.cpp#l4331 

http://hg.openjdk.java.net/jdk/jdk/file/0edc7fd0d7a3/src/java.base/share/classes/java/lang/Shutdown.java#l184 



If we want to avoid modifying the Java code, I think we can 
intercept JVM_BeforeHalt() and 
JavaThread::invoke_shutdown_hooks(). This way we should be 
able to handle all the classes (except those that are loaded 
inside 

Re: RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

[Brian Burkhalter]

Hi Roger,

Looks good!

Brian

> On Mar 6, 2020, at 8:16 AM, Roger Riggs  wrote:
> 
> Updated webrev:
> http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html 
> 
> 
> The threshold is a pretty loose target. 
> Given the original error that leaked a handle for every process started, it 
> just needs to detect a growing number of handles in use.  But for the sake of 
> accuracy and avoiding someone copying buggy code, its fixed.

Re: RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

[Roger Riggs]

Hi Naoto,

Updated webrev:
http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html

I don't think getting handle count of your own process can ever fail.
But in the abundance of caution, added a check and failure.

Copyright updated.

Thanks, Roger


Corrected copyright

On 3/5/20 4:14 PM, naoto.s...@oracle.com wrote:

Hi Roger,

In the test, the error value (-1) from the native code seems silently 
suppressed. Should it be caught/reported in the java side?


nit: copyright year in ProcessImpl.java -> 2020.

Naoto

On 3/5/20 12:51 PM, Roger Riggs wrote:
Please review a change to the Windows ProcessImpl to ensure that the 
handles
created for the input and output streams of a process are closed when 
no longer referenced.


Unlike on Linux, there is no thread monitoring the process that can 
close the streams.
The FileDescriptors are registered with the Cleaner to be closed when 
they are no longer referenced.


A test is added that monitors the count of handles as 50 Processes 
are launched and exit.

The test and change only affect the Windows implementation.

Webrev:
   http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/

Issue:
   https://bugs.openjdk.java.net/browse/JDK-8239893

Thanks, Roger

Re: RFR 8239893: Windows handle Leak when starting processes using ProcessBuilder

[Roger Riggs]

Hi Brian,

Updated webrev:
http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893-1/index.html

The threshold is a pretty loose target.
Given the original error that leaked a handle for every process started, 
it just needs to detect a growing number of handles in use.  But for the 
sake of accuracy and avoiding someone copying buggy code, its fixed.


Thanks, Roger


On 3/5/20 4:07 PM, Brian Burkhalter wrote:

Hi Roger,

On Mar 5, 2020, at 12:51 PM, Roger Riggs > wrote:


Please review a change to the Windows ProcessImpl to ensure that the 
handles
created for the input and output streams of a process are closed when 
no longer referenced.


Unlike on Linux, there is no thread monitoring the process that can 
close the streams.
The FileDescriptors are registered with the Cleaner to be closed when 
they are no longer referenced.


A test is added that monitors the count of handles as 50 Processes 
are launched and exit.

The test and change only affect the Windows implementation.

Webrev:
http://cr.openjdk.java.net/~rriggs/webrev-handles-8239893/

Issue:
https://bugs.openjdk.java.net/browse/JDK-8239893


In CheckHandles.java at line 72 there is this calculation:
final long ERROR_THRESHOLD = minHandles + (minHandles / 
ERROR_PERCENT); // 10% increase over min to passing max

Do you think it would be better as

final long ERROR_THRESHOLD = minHandles + ((minHandles + ERROR_PERCENT 
- 1) / ERROR_PERCENT); // 10% increase over min to passing max


, i.e., rounded instead of truncated?

Brian

PING: RFR: 8216407: java.util.UUID.fromString accepts input that does not match expected format

[Chihiro Ito]

Hi,

I tried to correct this problem.

Could you review this fix, please?

According to the RFC 4122, UUID has a fixed format. I tried to raise
an exception if a string was specified that is not suitable for this
format. Also, is there anything else I should be aware of with this
bug?

Webrev : http://cr.openjdk.java.net/~cito/JDK-8216407/webrev.00/
JBS : https://bugs.openjdk.java.net/browse/JDK-8216407

Regards,
Chihiro

2020年3月3日(火) 0:39 Chihiro Ito :
>
> Hi,
>
> I tried to correct this problem.
>
> Could you review this fix, please?
>
> According to the RFC 4122, UUID has a fixed format. I tried to raise an 
> exception if a string was specified that is not suitable for this format. 
> Also, is there anything else I should be aware of with this bug?
>
> Webrev : http://cr.openjdk.java.net/~cito/JDK-8216407/webrev.00/
> JBS : https://bugs.openjdk.java.net/browse/JDK-8216407
>
> Regards,
> Chihiro

Re: PING: RFR(s): 8240189: [TESTBUG] Some cgroup tests are failing after JDK-8231111

[Severin Gehwolf]

On Fri, 2020-03-06 at 14:52 +, Baesken, Matthias wrote:
> Hi Severin, looks good to me too .

Thanks for the review!

Cheers,
Severin

> Best regards, Matthias
> 
> 
> 
> > Hi,
> > 
> > I still need a *R*eview for this. Any takers?
> > 
> > Thanks,
> > Severin
> > 
> > On Fri, 2020-02-28 at 14:52 +0100, Severin Gehwolf wrote:
> > > Hi Bob,
> > > 
> > > On Thu, 2020-02-27 at 15:04 -0500, Bob Vandette wrote:
> > > > The updates look fine to me.
> > > 
> > > Thanks for the review!
> > > 
> > > >  Have you scanned through all tests looking for “.length” and [0] to
> > > > see if we have any more cases that need updating?
> > > 
> > > I have now and haven't found any places where it's incorrect. There are
> > > legit cases which still remain after this patch.
> > > 
> > > Thanks,
> > > Severin
> > > 
> > > > Bob.
> > > > 
> > > > 
> > > > > On Feb 27, 2020, at 2:31 PM, Severin Gehwolf 
> > > > > wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > Could somebody please review those test fixes for the Metrics
> > > > > platform
> > > > > tests? Some code paths are apparently not being executed on
> > > > > "common"
> > > > > platforms which didn't make those issues show up earlier :-/ I've
> > > > > missed to update some hard-coded values to account for changes done
> > > > > via
> > > > > JDK-823. Matthias Baesken from SAP was kind enough to run this
> > > > > through their testing and no more issues show up after this. Much
> > > > > appreciated, Matthias!
> > > > > 
> > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8240189
> > > > > webrev:
> > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> > 8240189/01/webrev/
> > > > > Testing: Docker tests on Linux x86_64 cgroups v1 and Linux cgroups
> > > > > v2
> > > > > (no change). Additional testing done by SAP. All seems to look
> > > > > good.
> > > > > 
> > > > > Thoughts?
> > > > > 
> > > > > Thanks,
> > > > > Severin
> > > > > 

RE: PING: RFR(s): 8240189: [TESTBUG] Some cgroup tests are failing after JDK-8231111

[Baesken, Matthias]

Hi Severin, looks good to me too .

Best regards, Matthias



> Hi,
> 
> I still need a *R*eview for this. Any takers?
> 
> Thanks,
> Severin
> 
> On Fri, 2020-02-28 at 14:52 +0100, Severin Gehwolf wrote:
> > Hi Bob,
> >
> > On Thu, 2020-02-27 at 15:04 -0500, Bob Vandette wrote:
> > > The updates look fine to me.
> >
> > Thanks for the review!
> >
> > >  Have you scanned through all tests looking for “.length” and [0] to
> > > see if we have any more cases that need updating?
> >
> > I have now and haven't found any places where it's incorrect. There are
> > legit cases which still remain after this patch.
> >
> > Thanks,
> > Severin
> >
> > > Bob.
> > >
> > >
> > > > On Feb 27, 2020, at 2:31 PM, Severin Gehwolf 
> > > > wrote:
> > > >
> > > > Hi,
> > > >
> > > > Could somebody please review those test fixes for the Metrics
> > > > platform
> > > > tests? Some code paths are apparently not being executed on
> > > > "common"
> > > > platforms which didn't make those issues show up earlier :-/ I've
> > > > missed to update some hard-coded values to account for changes done
> > > > via
> > > > JDK-823. Matthias Baesken from SAP was kind enough to run this
> > > > through their testing and no more issues show up after this. Much
> > > > appreciated, Matthias!
> > > >
> > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8240189
> > > > webrev:
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-
> 8240189/01/webrev/
> > > >
> > > > Testing: Docker tests on Linux x86_64 cgroups v1 and Linux cgroups
> > > > v2
> > > > (no change). Additional testing done by SAP. All seems to look
> > > > good.
> > > >
> > > > Thoughts?
> > > >
> > > > Thanks,
> > > > Severin
> > > >

Re: [8u] PING: RFR: 8213734: SAXParser.parse(File, ..) does not close resources when Exception occurs.

[Severin Gehwolf]

Hi Andrew,

On Tue, 2020-02-18 at 10:58 +, Andrew Hughes wrote:
> On 11/10/2019 15:54, Severin Gehwolf wrote:
> > On Fri, 2019-10-11 at 16:47 +0200, Mario Torre wrote:
> > > Hi Severin,
> > > 
> > > The patch looks good to me.
> > 
> > Thanks for the review, Mario!
> > 
> > Cheers,
> > Severin
> > 
> > > On Fri, Sep 27, 2019 at 10:38 AM Severin Gehwolf  
> > > wrote:
> > > > Hi!
> > > > 
> > > > I'd appreciate a review of this one. Thanks in advance! See below.
> > > > 
> > > > On Tue, 2019-09-03 at 17:50 +0200, Severin Gehwolf wrote:
> > > > > On Mon, 2019-08-26 at 17:54 +0200, Severin Gehwolf wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > Please review this 8u backport. The OpenJDK 11u patch does not apply
> > > > > > cleanly after path-reshuffeling due to a) test and code for jaxp are
> > > > > > split in JDK 8 b) Some rejects in comments - copyright and last
> > > > > > modified date. I've omitted rejected comments. I can manually add 
> > > > > > them
> > > > > > if that's preferred. See below.
> > > > > > 
> > > > > > Bug: https://bugs.openjdk.java.net/browse/JDK-8213734
> > > > > > webrevs:
> > > > > >   jdk:  
> > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213734/jdk8/jdk/01/webrev/
> > > > > >   jaxp: 
> > > > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213734/jdk8/jaxp/01/webrev/
> > > > > > 
> > > > > > Testing: tier1 on Linux x86_64. javax/xml/jaxp tests. New test on
> > > > > >  Windows without the fix fails and passes with it.
> > > > > > 
> > > > > > Thanks to Simon Tooke who helped testing this patch on Windows.
> > > > > > 
> > > > > > Rejects:
> > > > > > $ cat 
> > > > > > src/com/sun/org/apache/xerces/internal/impl/XMLEntityManager.java.rej
> > > > > > --- XMLEntityManager.java
> > > > > > +++ XMLEntityManager.java
> > > > > > @@ -1,5 +1,5 @@
> > > > > >  /*
> > > > > > - * Copyright (c) 2009, 2017, Oracle and/or its affiliates. All 
> > > > > > rights reserved.
> > > > > > + * Copyright (c) 2009, 2018, Oracle and/or its affiliates. All 
> > > > > > rights reserved.
> > > > > >   */
> > > > > >  /*
> > > > > >   * Licensed to the Apache Software Foundation (ASF) under one or 
> > > > > > more
> > > > > > @@ -89,7 +89,7 @@
> > > > > >   * @author K.Venugopal SUN Microsystems
> > > > > >   * @author Neeraj Bajaj SUN Microsystems
> > > > > >   * @author Sunitha Reddy SUN Microsystems
> > > > > > - * @LastModified: Oct 2017
> > > > > > + * @LastModified: Nov 2018
> > > > > >   */
> > > > > >  public class XMLEntityManager implements XMLComponent, 
> > > > > > XMLEntityResolver {
> > > > > > 
> > > > > 
> > > > > Gentle reminder.
> > > > 
> > > > Anyone? It's been a month :(
> > > > 
> > > > FWIW, I've updated XMLEntityManager to include the copyright update
> > > > upper bound to 2018. The @LastModified lines don't exist in JDK 8u, so
> > > > I've omitted that. Latest webrevs:
> > > > 
> > > > webrevs:
> > > >jdk:  
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213734/jdk8/jdk/01/webrev/
> > > >jaxp: 
> > > > http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8213734/jdk8/jaxp/02/webrev/
> > > > 
> > > > Thanks,
> > > > Severin
> > > > 
> 
> The copyright header was going to be my one comment about the JAXP
> patch, so good to see it fixed in the second revision. The @LastModified
> lines were added by "8193568: @LastModified tag in license header", an
> odd fix which is apparently not even viewable. Maybe someone will end up
> having to backport it just because it becomes such a pain :(
> 
> With the JDK side, you don't really explain why it was necessary to
> convert the test from a TestNG one. Could you elaborate please?

That was a long time ago. I don't quite remember, but it appears to
have been a test infra issue. For example BasePolicy doesn't seem to be
in 8. It came with JDK-8067170 which is a huge patch not really
suitable to backport.

Is that explanation sufficient?

Thanks,
Severin

Re: RFR 8240524: Removed warnings from test classes

[Vyom Tiwari]

Hi Vipin,

Test code changes looks good to me as well except copyright  changes, which
will be fix at the time of pushing the code by Christoph.

Thanks,
Vyom

On Fri, Mar 6, 2020 at 12:06 PM Langer, Christoph 
wrote:

> Hi Vipin,
>
> this all looks correct to me.
>
> When changing the copyright headers, you have to keep the first (initial)
> year and only update the second year. If this doesn’t exist, you’ll have to
> add it, e.g.
> Copyright (c) 1999, Oracle -> Copyright (c) 1999, 2020, Oracle
>
> I can fix that for you though, when sponsoring, no need for new webrev.
>
> Can I have a second review, please?
>
> Thanks
> Christoph
>
>
>
> From: Vipin Sharma 
> Sent: Donnerstag, 5. März 2020 18:27
> To: Langer, Christoph ;
> core-libs-dev@openjdk.java.net
> Subject: RFR 8240524: Removed warnings from test classes
>
> Hi All,
>
> Please review patch to remove warnings from test classes.
>
> Bug : https://bugs.openjdk.java.net/browse/JDK-8240524
> Webrev : http://cr.openjdk.java.net/~clanger/webrevs/8240524.0/
>
>
> Change description:
>
> Class: test/jdk/java/lang/Boolean/GetBoolean.java
> Fixed following warning:
> 1. "Exception 'java.lang.Exception' is never thrown in the method”
>
> Class: test/jdk/java/lang/Boolean/MakeBooleanComparable.java
> Fixed following warnings:
> 1. Explicit type argument Boolean can be replaced with <>
> 2. C-style array declaration of parameter 'args'
>
> Class: test/jdk/java/lang/Boolean/ParseBoolean.java
> Fixed following warning:
> 1. Exception 'java.lang.Exception' is never thrown in the method
>
>
> Regards,
> Vipin
>


-- 
Thanks,
Vyom