RFR: implementation for JEP 334: JVM Constants API

[Vicente Romero]

Hi all,

Please review the proposed implementation for JEP 334 [1]. The webrev 
can be found at [2] and the javadoc at [3].


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8203252
[2] 
http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.patch
[3] 
http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary.html


PS. We are offering a MacBook Wheel to the authors of the first 5 
comments :)

Re: RFR: implementation for JEP 334: JVM Constants API

[Vicente Romero]

Thanks for the comments so far, I have uploaded another iteration of the 
implementation + javadoc


Vicente

[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.08
[2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.08


On 05/23/2018 02:41 PM, Vicente Romero wrote:

Hi all,

Please review the proposed implementation for JEP 334 [1]. The webrev 
can be found at [2] and the javadoc at [3].


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8203252
[2] 
http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.patch
[3] 
http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary.html


PS. We are offering a MacBook Wheel to the authors of the first 5 
comments :)

Re: RFR: implementation for JEP 334: JVM Constants API

[Vicente Romero]

Hi all,

I have uploaded another iteration of the implementation of the constants 
API + the javadoc.


Thanks for the comments so far,
Vicente


[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.09
[2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.09

On 05/24/2018 09:09 PM, Vicente Romero wrote:
Thanks for the comments so far, I have uploaded another iteration of 
the implementation + javadoc


Vicente

[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.08
[2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.08


On 05/23/2018 02:41 PM, Vicente Romero wrote:

Hi all,

Please review the proposed implementation for JEP 334 [1]. The webrev 
can be found at [2] and the javadoc at [3].


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8203252
[2] 
http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.patch
[3] 
http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary.html


PS. We are offering a MacBook Wheel to the authors of the first 5 
comments :)

Re: RFR: implementation for JEP 334: JVM Constants API

[Vicente Romero]

Hi all,

There is a new iteration of the implementation at:

[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.10
[2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.10

main changes in this iteration: we have added additional tests to 
improve code coverage.


Thanks,
Vicente


On 05/31/2018 05:09 PM, Vicente Romero wrote:

Hi all,

I have uploaded another iteration of the implementation of the 
constants API + the javadoc.


Thanks for the comments so far,
Vicente


[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.09
[2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.09

On 05/24/2018 09:09 PM, Vicente Romero wrote:
Thanks for the comments so far, I have uploaded another iteration of 
the implementation + javadoc


Vicente

[1] http://cr.openjdk.java.net/~vromero/constant.api/webrev.08
[2] http://cr.openjdk.java.net/~vromero/constant.api/javadoc.08


On 05/23/2018 02:41 PM, Vicente Romero wrote:

Hi all,

Please review the proposed implementation for JEP 334 [1]. The 
webrev can be found at [2] and the javadoc at [3].


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8203252
[2] 
http://cr.openjdk.java.net/~vromero/constant.api/webrev.07/constants.api.patch
[3] 
http://cr.openjdk.java.net/~vromero/constant.api/javadoc.07/overview-summary.html


PS. We are offering a MacBook Wheel to the authors of the first 5 
comments :)

Re: RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present

[Vicente Romero]

I'm not an expert in the area, but the patch looks good to me,

Vicente

On 05/31/2018 08:39 PM, Liam Miller-Cushon wrote:

Hi,

Are there any concerns with the patch that I can address? I was hoping to
get this in to JDK 11.

I confirmed that it still builds and passes all tests at head.

Thanks,
Liam

On Mon, May 14, 2018 at 10:38 AM Liam Miller-Cushon 
wrote:


Ping

On Thu, Apr 5, 2018 at 10:57 AM Liam Miller-Cushon 
wrote:


Hi,

Is there any more feedback on the patch?

On Wed, Feb 28, 2018 at 4:26 PM Liam Miller-Cushon 
wrote:


On Wed, Feb 28, 2018 at 4:13 PM, Martin Buchholz 
wrote:


Follow their lead and rename your method to assertArrayEquals


Done: http://cr.openjdk.java.net/~cushon/7183985/webrev.04/

Re: RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

adding core-libs in the loop

On 10/10/2018 12:30 PM, Vicente Romero wrote:

Hi all,

I have updated the webrev [1], this version removes the `implements 
Constable` from the symbolic descriptor classes. Feedback is mostly 
appreciated,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.01/jdk12.dev.patch

On 10/06/2018 05:00 PM, Brian Goetz wrote:
What we decided to do here is to hold back on “implements Constable” 
for the symbolic descriptor classes in the initial push of JEP-334, 
and then when we have the symbolic expression mode for BSMs, 
re-implement those in XxxDesc using that.  Implementing Constable 
isn’t needed until we get to the full constant folding anyway.  That 
linearizes the dependencies — first JEP-334, then symbolic mode BSM, 
then update JEP-334 classes to implement Constable using symbolic 
mode BSM.



On Sep 13, 2018, at 9:07 PM, John Rose  wrote:

I am running a review of VM-level work on bootstrap methods
which can optionally help simplify some of these APIs:

http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-September/030084.html 



Specifically, this work can be use to implement a "symbolic
expression mode" for BSMs which causes the JVM to unpack
constant pool nodes directly as ConstantDesc items to present
to BSMs.  This might simplify the condy forms of ConstantDesc
instances, if javac stores native constants to reflect, rather than
lists of strings to reassemble.

— John

On Sep 11, 2018, at 12:50 PM, Vicente Romero 
 wrote:
Please review the first iteration of the implementation for JEP-334 
[1] JVM Constants API. The implementation can be found at [2]. 
JEP-334 introduces an API to model nominal descriptions of key 
class-file and run-time artifacts, in particular constants that are 
loadable from the constant pool and has already been the subject of 
several discussions. The implementation of this JEP has been 
publicly accessible throw the amber repo at [3] in the jep-334 
branch. Thanks to all members of the Amber project and specially to 
Brian for all the hard work on the design and the implementation of 
this API. Thanks for all the feedback we have received so far, most 
of it has been integrated in the current implementation.


Thanks,
Vicente

[1] http://openjdk.java.net/jeps/334
[2] 
http://cr.openjdk.java.net/~vromero/8210031/webrev.00/jdk.dev.patch

[3] http://hg.openjdk.java.net/amber/amber

Re: RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi all,

sorry for the repeated number of mails on this issue. I have added a 
direct link to the patch the right link to the webrev is [1] there is a 
previous version at [2] if you want to see the differences with the last 
version. Basically we have dropped the `implements Constable` for the 
symbolic descriptor classes and removed all of the code that was useful 
for constant folding but not strictly necessary for the API. For more 
information on the JEP see [3]


[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.01
[2] http://cr.openjdk.java.net/~vromero/8210031/webrev.00
[3] http://openjdk.java.net/jeps/334

On 10/15/2018 01:51 PM, Vicente Romero wrote:

adding core-libs in the loop

On 10/10/2018 12:30 PM, Vicente Romero wrote:

Hi all,

I have updated the webrev [1], this version removes the `implements 
Constable` from the symbolic descriptor classes. Feedback is mostly 
appreciated,


Thanks,
Vicente

[1] 
http://cr.openjdk.java.net/~vromero/8210031/webrev.01/jdk12.dev.patch


On 10/06/2018 05:00 PM, Brian Goetz wrote:
What we decided to do here is to hold back on “implements Constable” 
for the symbolic descriptor classes in the initial push of JEP-334, 
and then when we have the symbolic expression mode for BSMs, 
re-implement those in XxxDesc using that.  Implementing Constable 
isn’t needed until we get to the full constant folding anyway.  That 
linearizes the dependencies — first JEP-334, then symbolic mode BSM, 
then update JEP-334 classes to implement Constable using symbolic 
mode BSM.



On Sep 13, 2018, at 9:07 PM, John Rose  wrote:

I am running a review of VM-level work on bootstrap methods
which can optionally help simplify some of these APIs:

http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-September/030084.html 



Specifically, this work can be use to implement a "symbolic
expression mode" for BSMs which causes the JVM to unpack
constant pool nodes directly as ConstantDesc items to present
to BSMs.  This might simplify the condy forms of ConstantDesc
instances, if javac stores native constants to reflect, rather than
lists of strings to reassemble.

— John

On Sep 11, 2018, at 12:50 PM, Vicente Romero 
 wrote:
Please review the first iteration of the implementation for 
JEP-334 [1] JVM Constants API. The implementation can be found at 
[2]. JEP-334 introduces an API to model nominal descriptions of 
key class-file and run-time artifacts, in particular constants 
that are loadable from the constant pool and has already been the 
subject of several discussions. The implementation of this JEP has 
been publicly accessible throw the amber repo at [3] in the 
jep-334 branch. Thanks to all members of the Amber project and 
specially to Brian for all the hard work on the design and the 
implementation of this API. Thanks for all the feedback we have 
received so far, most of it has been integrated in the current 
implementation.


Thanks,
Vicente

[1] http://openjdk.java.net/jeps/334
[2] 
http://cr.openjdk.java.net/~vromero/8210031/webrev.00/jdk.dev.patch

[3] http://hg.openjdk.java.net/amber/amber

Re: RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi all,

Please review also the CSR at [1].

Thanks,
Vicente

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

On 10/15/18 2:12 PM, Vicente Romero wrote:

Hi all,

sorry for the repeated number of mails on this issue. I have added a 
direct link to the patch the right link to the webrev is [1] there is 
a previous version at [2] if you want to see the differences with the 
last version. Basically we have dropped the `implements Constable` for 
the symbolic descriptor classes and removed all of the code that was 
useful for constant folding but not strictly necessary for the API. 
For more information on the JEP see [3]


[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.01
[2] http://cr.openjdk.java.net/~vromero/8210031/webrev.00
[3] http://openjdk.java.net/jeps/334

On 10/15/2018 01:51 PM, Vicente Romero wrote:

adding core-libs in the loop

On 10/10/2018 12:30 PM, Vicente Romero wrote:

Hi all,

I have updated the webrev [1], this version removes the `implements 
Constable` from the symbolic descriptor classes. Feedback is mostly 
appreciated,


Thanks,
Vicente

[1] 
http://cr.openjdk.java.net/~vromero/8210031/webrev.01/jdk12.dev.patch


On 10/06/2018 05:00 PM, Brian Goetz wrote:
What we decided to do here is to hold back on “implements 
Constable” for the symbolic descriptor classes in the initial push 
of JEP-334, and then when we have the symbolic expression mode for 
BSMs, re-implement those in XxxDesc using that.  Implementing 
Constable isn’t needed until we get to the full constant folding 
anyway. That linearizes the dependencies — first JEP-334, then 
symbolic mode BSM, then update JEP-334 classes to implement 
Constable using symbolic mode BSM.


On Sep 13, 2018, at 9:07 PM, John Rose  
wrote:


I am running a review of VM-level work on bootstrap methods
which can optionally help simplify some of these APIs:

http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2018-September/030084.html 



Specifically, this work can be use to implement a "symbolic
expression mode" for BSMs which causes the JVM to unpack
constant pool nodes directly as ConstantDesc items to present
to BSMs.  This might simplify the condy forms of ConstantDesc
instances, if javac stores native constants to reflect, rather than
lists of strings to reassemble.

— John

On Sep 11, 2018, at 12:50 PM, Vicente Romero 
 wrote:
Please review the first iteration of the implementation for 
JEP-334 [1] JVM Constants API. The implementation can be found at 
[2]. JEP-334 introduces an API to model nominal descriptions of 
key class-file and run-time artifacts, in particular constants 
that are loadable from the constant pool and has already been the 
subject of several discussions. The implementation of this JEP 
has been publicly accessible throw the amber repo at [3] in the 
jep-334 branch. Thanks to all members of the Amber project and 
specially to Brian for all the hard work on the design and the 
implementation of this API. Thanks for all the feedback we have 
received so far, most of it has been integrated in the current 
implementation.


Thanks,
Vicente

[1] http://openjdk.java.net/jeps/334
[2] 
http://cr.openjdk.java.net/~vromero/8210031/webrev.00/jdk.dev.patch

[3] http://hg.openjdk.java.net/amber/amber

Re: RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi all,

Thanks Mandy for the review. I have uploaded a new iteration [1].

Vicente

[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.02/

On 10/18/18 9:55 PM, Mandy Chung wrote:



On 10/15/18 11:12 AM, Vicente Romero wrote:


[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.01


I reviewed java.lang.invoke change in details.  I have skimmed through 
the new classes.

I will look at the new tests next.

@since 12 is missing in the new APIs

VarHandle.java
1887 public final String toString() {
1888 // @@@ defer to concrete type for additional description
1889 // see https://bugs.openjdk.java.net/browse/JDK-8199149 You may 
want to take out this comment or L1889 as we can refer back to 
JDK-8199149. VarHandles.java
 169 // @@@ This is a little fragile assuming the base is the 
class


Maybe FieldStaticReadOnly and FieldStaticReadWrite constructor and
getStaticFieldFromBaseAndOffset method should take Class refc
rather than Object base. FieldStaticReadXXX will do the cast when
calling getStaticFieldFromBaseAndOffset.

java.base module-info.java
   It'd be good to keep the exported APIs in alphabetical order.

java/lang/invoke/TypeDescriptor.java
   copyright header is missing

Mandy

Re: JDK 12 RFR of JDK-6304578: (reflect) toGenericString fails to print bounds of type variables on generic methods

[Vicente Romero]

last iteration looks good to me,

Vicente

On 10/25/18 5:47 PM, joe darcy wrote:

Hi Peter,

Coming back to this review after my Code One activities this year have 
run their course...



On 10/17/2018 3:07 PM, Peter Levart wrote:

Hi Joe,

On 10/17/2018 09:16 PM, joe darcy wrote:
PS In response to some off-list feedback, an updated webrev uses a 
stream-ier implementation:


http://cr.openjdk.java.net/~darcy/6304578.1/


I suggest further niceing:

- if you make typeVarBounds() static, you could use it in 
Stream.map() as a method reference.
- in Class.java line 285, you could also use Type::getTypeName method 
reference
- in Class.java line 269, wouldn't the output be nicer if the 
delimiter in joining collector was ", " instead of "," (with a space 
after comma)?


The same for Executable.



Revised webrev generally taking those suggestions up at:

    http://cr.openjdk.java.net/~darcy/6304578.2/

Diff of relevant parts of prior version below (screening out unrelated 
changes to Class made in the interim).


The behavior with respect to using a comma as a separator is part of 
the spec of these methods and I don't want to alter that as part of 
this change.


Thanks,

-Joe

diff -r 6304578.1/raw_files/new/  6304578.2/raw_files/new/
diff -r 
6304578.1/raw_files/new/src/java.base/share/classes/java/lang/Class.java 
6304578.2/raw_files/new/src/java.base/share/classes/java/lang/Class.java

268c268
< sb.append(Stream.of(typeparms).map(t -> typeVarBounds(t)).
---
> sb.append(Stream.of(typeparms).map(Class::typeVarBounds).
279c279
< String typeVarBounds(TypeVariable typeVar) {
---
> static String typeVarBounds(TypeVariable typeVar) {
285c285
< Stream.of(bounds).map(e -> e.getTypeName()).
---
> Stream.of(bounds).map(Type::getTypeName).
3413c3413,3414
< StringJoiner sj = new StringJoiner(", ", getName() + "." + 
name + "(", ")");

---
> StringBuilder sb = new StringBuilder();
> sb.append(getName() + "." + name + "(");
3415,3420c3416,3420
< for (int i = 0; i < argTypes.length; i++) {
< Class c = argTypes[i];
< sj.add((c == null) ? "null" : c.getName());
< }
< }
< return sj.toString();
---
> Stream.of(argTypes).map(c -> {return (c == null) ? "null" : 
c.getName();}).

> collect(Collectors.joining(","));
> }
> sb.append(")");
> return sb.toString();
diff -r 
6304578.1/raw_files/new/src/java.base/share/classes/java/lang/reflect/Executable.java 
6304578.2/raw_files/new/src/java.base/share/classes/java/lang/reflect/Executable.java 


116c116
< sb.append(Stream.of(parameterTypes).map(p -> p.getTypeName()).
---
> sb.append(Stream.of(parameterTypes).map(Type::getTypeName).
122c122
< sb.append(Stream.of(exceptionTypes).map(e -> e.getTypeName()).
---
> sb.append(Stream.of(exceptionTypes).map(Type::getTypeName).
137c137
< String typeVarBounds(TypeVariable typeVar) {
---
> static String typeVarBounds(TypeVariable typeVar) {
143c143
< Stream.of(bounds).map(e -> e.getTypeName()).
---
> Stream.of(bounds).map(Type::getTypeName).
156c156
< sb.append(Stream.of(typeparms).map(t -> typeVarBounds(t)).
---
> sb.append(Stream.of(typeparms).map(Executable::typeVarBounds).
176,180c176,177
< StringJoiner joiner = new StringJoiner(",", " throws 
", "");

< for (Type exceptionType : exceptionTypes) {
< joiner.add(exceptionType.getTypeName());
< }
< sb.append(joiner.toString());
---
> sb.append(Stream.of(exceptionTypes).map(Type::getTypeName).
>   collect(Collectors.joining(",", " throws ", "")));

RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi,

Version 7.0 of ASM has been released. This version supports condy, yay!, 
and we want to include it in JDK 12. Please review [1] which includes:
- the new version perse substituting the preview ASM internal version in 
the JDK
- changes to additional files in particular some tests, mostly hotspot 
tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi Igor,

On 11/7/18 2:33 PM, Igor Ignatyev wrote:

Hi Vicente,

I recall an (internal?) discussion about updating ASM somewhen in JDK 
11TF, and AFAIR it was decided not to update ASM b/c nothing in JDK 
needs that, has it been changed? put somewhat differently, why are we 
doing this?


we need the support ASM 7 has of condy, many amber projects depend on this



in any case, I don't like the changes in mlvm tests. I understand 
that ClassWriter has been significantly changed in ASM 7.0, 
so ClassWriterExt can't disable CP entries caching (at least not in 
the way it used to), but removing setCache* calls from the tests 
changed them and in some cases made them invalid as they don't test 
that they supposed to. therefore I'd prefer to leave all calls 
setCache* as-is, change setCache* implementation to throw an exception 
(similarly to the fix in JDK-8194826 
<https://bugs.openjdk.java.net/browse/JDK-8194826>) and mark all tests 
which throw this exception w/ '@ignore 8194951' jtreg tag.


sounds good, I will do this



Thanks,
-- Igor


Thanks,
Vicente



On Nov 7, 2018, at 7:56 AM, Vicente Romero <mailto:vicente.rom...@oracle.com>> wrote:


Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal version 
in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

On 11/8/18 8:14 AM, Alan Bateman wrote:

On 07/11/2018 19:33, Igor Ignatyev wrote:

Hi Vicente,

I recall an (internal?) discussion about updating ASM somewhen in JDK 
11TF, and AFAIR it was decided not to update ASM b/c nothing in JDK 
needs that, has it been changed? put somewhat differently, why are we 
doing this?


in any case, I don't like the changes in mlvm tests. I understand 
that ClassWriter has been significantly changed in ASM 7.0, so 
ClassWriterExt can't disable CP entries caching (at least not in the 
way it used to), but removing setCache* calls from the tests changed 
them and in some cases made them invalid as they don't test that they 
supposed to. therefore I'd prefer to leave all calls setCache* as-is, 
change setCache* implementation to throw an exception (similarly to 
the fix in JDK-8194826 
) and mark all 
tests which throw this exception w/ '@ignore 8194951' jtreg tag.



ClassWriterExt the MLVM tests have come in previous upgrades too. Has 
there been any discussion Remi or others on ASM to make it easier for 
the JDK to upgrade?


I'm not aware of any such discussions.



-Alan


Vicente

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features?


I changed only the tests for which the new ASM was complaining about a 
particular API available only for ASM7


If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with ASM5).


I have made two webrevs to make the review easier [1], contain only the 
changes to the internal asm and [2] contains the changes to the clients 
plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/


On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal version 
in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi David,

On 11/8/18 11:00 PM, David Holmes wrote:

Hi Vicente,

On 9/11/2018 2:39 AM, Vicente Romero wrote:

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible 
to see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ 
change to specifying Opcodes.ASM7 unless they plan on using ASM7 
features?


I changed only the tests for which the new ASM was complaining about 
a particular API available only for ASM7


I could not understand how this could be if the tests and other code 
were unchanged, so I applied the ASM-only patch and investigated the 
first failure running nasgen. And of course the problem is the 
NestHost/NestMembers attributes! We modified our internal version of 
ASM to add nestmate support, but of course this update removes that 
and replaces it with the official support. But the official support is 
only enabled for ASMv7 so we must update all users of ASM to request 
version 7.


thanks for double checking this


Thanks,
David
-


Vicente



If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with 
ASM5).


I have made two webrevs to make the review easier [1], contain only 
the changes to the internal asm and [2] contains the changes to the 
clients plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/ 



On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal 
version in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

any other comment after the last iteration? we are in a bit of a hurry 
to push this before the JDK 12 train departs :(


Thanks,
Vicente

On 11/8/18 11:39 AM, Vicente Romero wrote:

Hi David, Igor

On 11/7/18 10:03 PM, David Holmes wrote:

Hi Vicente,

All of the javadoc comment reformatting makes it nearly impossible to 
see the actual substantive changes :(


ASM 7 also supports the Nestmate attributes and I was trying to see 
how/where that appeared but its somewhat obscure. Oh well.


Is it that case that the code the uses the ASM library, like the JFR 
code and jlink code, and the tests, doesn't actually _have to_ change 
to specifying Opcodes.ASM7 unless they plan on using ASM7 features?


I changed only the tests for which the new ASM was complaining about a 
particular API available only for ASM7


If so then you could split out the actual update of ASM from the 
updates to the users of ASM (some of which may be quite fine with ASM5).


I have made two webrevs to make the review easier [1], contain only 
the changes to the internal asm and [2] contains the changes to the 
clients plus make files, legal, etc. I have also made the changes to 
ClassWriterExt and affected test proposed by Igor in another mail,




Thanks,
David


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/


On 8/11/2018 1:56 AM, Vicente Romero wrote:

Hi,

Version 7.0 of ASM has been released. This version supports condy, 
yay!, and we want to include it in JDK 12. Please review [1] which 
includes:
- the new version perse substituting the preview ASM internal 
version in the JDK
- changes to additional files in particular some tests, mostly 
hotspot tests.


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.00/

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi Alan,

On 11/13/18 9:18 AM, Alan Bateman wrote:

On 13/11/2018 14:00, Vicente Romero wrote:
any other comment after the last iteration? we are in a bit of a 
hurry to push this before the JDK 12 train departs :(
The original patch updated all the use sites (and tests) to specify 
ASM7 for the API version. I just checked the webrev again now and it 
seems to be just the ASM refresh now. Assuming all the tests are 
passing and you've sorted out the mvlm test issues with Igor then I 
suggest go ahead with this push and we can update the sites, as 
needed, at a later time.


in the last update I sent links to two patches [1] is the ASM7 only 
changes and [2] is the changes to the use sites. My plan is to push both 
together, but I split them to ease the review process. But still I will 
get your go and push it as good ;)




-Alan


Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/ 

Re: RFR: JDK-8213480: update internal ASM version to 7.0

[Vicente Romero]

Hi Igor,

Thanks for your comment. I have already applied in my local copy.

Vicente

On 11/13/18 8:30 PM, Igor Ignatyev wrote:

Hi Vicente,

you need to replace "@ignore 8194951: some mlvm tests fail w/ ASMv7" w/ "@ignore 
8194951" in all the occurrences, as we have monitoring tools which expect @ignore to be 
followed by a space-separated list of bug ids. the rest looks good to me.

Thanks,
-- Igor


On Nov 13, 2018, at 5:11 PM, Vicente Romero  wrote:

Hi Alan,

On 11/13/18 9:18 AM, Alan Bateman wrote:

On 13/11/2018 14:00, Vicente Romero wrote:

any other comment after the last iteration? we are in a bit of a hurry to push 
this before the JDK 12 train departs :(

The original patch updated all the use sites (and tests) to specify ASM7 for 
the API version. I just checked the webrev again now and it seems to be just 
the ASM refresh now. Assuming all the tests are passing and you've sorted out 
the mvlm test issues with Igor then I suggest go ahead with this push and we 
can update the sites, as needed, at a later time.

in the last update I sent links to two patches [1] is the ASM7 only changes and 
[2] is the changes to the use sites. My plan is to push both together, but I 
split them to ease the review process. But still I will get your go and push it 
as good ;)


-Alan

Vicente

[1] http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.only.00/
[2] 
http://cr.openjdk.java.net/~vromero/8213480/webrev.asm.7.additional.changes.00/

JDK12 talks ASM7 now!

[Vicente Romero]

Hi guys,

Thanks everyone for the feedback. I pushed the patch yesterday,

Vicente

Re: Reflection, TYPE_USE annotation on THROWS on inner class constructor, Java 9+

[Vicente Romero]

Hi Michael,

Thanks for the report. I have created: 
https://bugs.openjdk.java.net/browse/JDK-8213905 to track it,


Vicente

On 11/9/18 10:13 AM, Michael Rasmussen wrote:

Hi

When adding a TYPE_USE annotation with runtime retention to the throws 
exception clause of a constructor of an inner class, these annotations are not 
found via Constructor.getAnnotatedExceptionType().getDeclaredAnnotations() on 
Java 9+, but present when running on Java 8.
For static nested classes, the annotations are correctly found on Java 9+.

See sample code below.

Looking in the .class file for the nested classes (with javap), the runtime 
annotation is present there:
   public app1.MiscTest8$Inner(app1.MiscTest8) throws java.lang.Exception;
 (...)
 RuntimeVisibleTypeAnnotations:
   0: #17(): THROWS, type_index=0
 app1.MiscTest8$MyAnnotation

and
   public app1.MiscTest8$StaticInner() throws java.lang.Exception;
 (...)
 RuntimeVisibleTypeAnnotations:
   0: #14(): THROWS, type_index=0
 app1.MiscTest8$MyAnnotation

When I run it on Java 8, I get the following output:
=== public app1.MiscTest8$Inner(app1.MiscTest8) throws java.lang.Exception ===
[@app1.MiscTest8$MyAnnotation()]
=== public app1.MiscTest8$StaticInner() throws java.lang.Exception ===
[@app1.MiscTest8$MyAnnotation()]
=== public static void app1.MiscTest8.main(java.lang.String[]) throws 
java.lang.Exception ===
[@app1.MiscTest8$MyAnnotation()]

But when I run the same class files on Java 9, 10, 11, (and 12-ea) I get:
=== public app1.MiscTest8$Inner(app1.MiscTest8) throws java.lang.Exception ===
[]
=== public app1.MiscTest8$StaticInner() throws java.lang.Exception ===
[@app1.MiscTest8$MyAnnotation()]
=== public static void app1.MiscTest8.main(java.lang.String[]) throws 
java.lang.Exception ===
[@app1.MiscTest8$MyAnnotation()]


Shouldn't I be able to find this annotation vis getAnnotatedExceptionTypes() on 
Java9+, similar to how it works on Java 8?

Kind regards
Michael Rasmussen.


/* --- snip --- */
package app1;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;
import java.lang.reflect.AnnotatedType;
import java.lang.reflect.Executable;
import java.util.Arrays;

public class MiscTest8 {

   @Retention(value = RetentionPolicy.RUNTIME)
   @Target(ElementType.TYPE_USE)
   @interface MyAnnotation {
   }

   class Inner {
 public Inner() throws @MyAnnotation Exception {
 }
   }

   static class StaticInner {
 public StaticInner() throws @MyAnnotation Exception {
 }
   }
   public static void main(String[] args) throws @MyAnnotation Exception {
 foo(Inner.class.getDeclaredConstructor(MiscTest8.class));
 foo(StaticInner.class.getDeclaredConstructor());
 foo(MiscTest8.class.getDeclaredMethod("main", String[].class));
   }

   static void foo(Executable c) throws Exception {
 System.out.println("=== " + c + " ===");

 for (AnnotatedType exceptionType : c.getAnnotatedExceptionTypes()) {
   
System.out.println(Arrays.toString(exceptionType.getDeclaredAnnotations()));
 }
   }
}
/* --- snap --- */

Re: RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi all,

Can I have the final nod to the JVM constants API, there have been some 
changes since the last review iteration. Thanks to the internal and 
external developers that have taken the time to provide feedback so far. 
The links to the last versions are:


webrev: http://cr.openjdk.java.net/~vromero/8210031/webrev.10/
javadoc: 
http://cr.openjdk.java.net/~vromero/8210031/javadoc.18/overview-summary.html
specdiff: 
http://cr.openjdk.java.net/~vromero/8210031/specdiff.08/overview-summary.html


Thanks,
Vicente


On 10/18/18 9:55 PM, Mandy Chung wrote:



On 10/15/18 11:12 AM, Vicente Romero wrote:


[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.01


I reviewed java.lang.invoke change in details.  I have skimmed through 
the new classes.

I will look at the new tests next.

@since 12 is missing in the new APIs

VarHandle.java
1887 public final String toString() {
1888 // @@@ defer to concrete type for additional description
1889 // see https://bugs.openjdk.java.net/browse/JDK-8199149 You may 
want to take out this comment or L1889 as we can refer back to 
JDK-8199149. VarHandles.java
 169 // @@@ This is a little fragile assuming the base is the 
class


Maybe FieldStaticReadOnly and FieldStaticReadWrite constructor and
getStaticFieldFromBaseAndOffset method should take Class refc
rather than Object base. FieldStaticReadXXX will do the cast when
calling getStaticFieldFromBaseAndOffset.

java.base module-info.java
   It'd be good to keep the exported APIs in alphabetical order.

java/lang/invoke/TypeDescriptor.java
   copyright header is missing

Mandy

Re: RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi Mandy,

On 12/5/18 1:12 PM, Mandy Chung wrote:



On 12/3/18 11:12 AM, Vicente Romero wrote:

Hi all,

Can I have the final nod to the JVM constants API, there have been 
some changes since the last review iteration. Thanks to the internal 
and external developers that have taken the time to provide feedback 
so far. The links to the last versions are:


webrev: http://cr.openjdk.java.net/~vromero/8210031/webrev.10/
javadoc: 
http://cr.openjdk.java.net/~vromero/8210031/javadoc.18/overview-summary.html
specdiff: 
http://cr.openjdk.java.net/~vromero/8210031/specdiff.08/overview-summary.html






Thanks for your suggestions I have fixed them.

I reviewed webrev.10 and overall looks good to me.  A couple of minor 
comments and some of them seems to be fixed already in amber repo.  No 
need to generate a new webrev.


Nit: The javadoc of the new methods starts with "Returns", "Return", 
"Produce", "Create", "Resolve", "Compares" etc.  It'd be good to do a 
pass and fix the verb form consistently.


src/java.base/share/classes/java/lang/Class.java
   nit: Line 163 seems to have extra white-spaces, not aligned with 
the other superinterfaces.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java 
74 private static final Set suppressSubtypesSet

75 = Set.of("java.lang.Object",
76 "org.omg.CORBA.Object");

This is not related to your change. CORBA is no longer in JDK.
Maybe this special casing is no longer needed.  It may worth
filing a JBS issue to examine this.

ConstantDescs.java
   64 // Don't change the order of these declarations!

Is there any code depending on this order?


not really the order is in increasing `complexity` but there is no hard 
dependency on it just to keep a logical order




DirectMethodHandleDesc.java
   46  * {@link MethodHandle}.  A {@linkplain DirectMethodHandleDescImpl} 
corresponds to

typo: DirectMethodHandleDescImpl

   57 /**
   58  * Kinds of method handles that can be described with {@linkplain 
DirectMethodHandleDesc}.
   59  */
   60 enum Kind {

This needs @since 12

DynamicCallSiteDesc.java
   59  * @param bootstrapMethod a {@link DirectMethodHandleDescImpl} 
describing the
   60  *bootstrap method for the {@code 
invokedynamic}

typo: DirectMethodHandleDescImpl and in a few other @param

Mandy

Thanks,
Vicente

Re: RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi Roger,

Thanks for your comments, see my comments below. I have published 
another iteration at [1]


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.12/

On 12/5/18 4:35 PM, Roger Riggs wrote:

Hi,

Editorial comments java.lang.contant package javadoc:

 - In the Nominal Descriptors  2nd paragraph:  "of of" -> "of"

-  DirectMethodHandleDescImpl - Its unusual to see a Class named *Impl in
   a developer facing API.  And if its intended, it should be linked.


DirectMethodHandleDescImpl is not public



- "These classes provides" plural agreement.


DynamicCallSiteDesc:

 - Is the format of names clearly specified:  IAE -> if the invocation 
name has the 'incorrect format'.


I think so, depending on the case it the related part of the JVMS is 
added as a reference




 - typo:   "descripbing" - > "describing"

 - missing @throws IAE for method :

   ​of(DirectMethodHandleDesc
<http://cr.openjdk.java.net/%7Evromero/8210031/javadoc.18/java/lang/constant/DirectMethodHandleDesc.html> 
 bootstrapMethod,String
<http://cr.openjdk.java.net/%7Evromero/8210031/javadoc.18/java/lang/String.html> 
 invocationName,MethodTypeDesc
<http://cr.openjdk.java.net/%7Evromero/8210031/javadoc.18/java/lang/constant/MethodTypeDesc.html> 
 invocationType)



- The withArgs method only throws NPE:  is there nothing else that can 
go wrong?
the rest of the arguments passed to the invocation to constructor 
DynamicCallSiteDesc has already been checked so no other exceptions 
associated to them should be thrown


- bootStrapArgs() - the return is always non-null, with zero length 
array for non args?


it has to be, IMO, as the private constructor that is the only one that 
assigns a value to field bootstrapArgs makes sure that the assigned 
value is non-null




DynamicConstantDesc.java:
 - First sentences should end with period. "."

 - ofCanonical:  2nd sentence, "Classes ... produces ... a 
ConstantDesc"; grammar?


 - of Canaonical(): So a conforming implementation is not required to 
return the

    well known values, only suggested.

ClassDesc.java:
 - The create methods should not be required to create new instances.  
Use "return"  instead of "create".


 - How are class descriptors created for 'nested' classes (as opposed 
to inner classes).

   It is worth describing or referencing how that should be done.


not sure I get what you mean. If you want to create a class descriptor 
for a nested class you should be able to invoke inner() with an string 
containing a number, what should be made more clear here in your opinion?




 - Can inner() throw IAE for invalid format names?

yes added


 - How can the rank of an array ClassDesc be found?


we can add a method for this after the integration



Constable.java:

 - Avoid ending the first sentence of describeConstable description 
with "be".


ConstDesc.java:
 - "with respect to a particular to a class loader" ; extra words "to a"?

DirectMethodHandleDesc.java:
 - ofField, @throws IAE might mention the possibility of an invalid name


right


MethodTypeDesc.java:
 - First sentences should end with period. "."

Regards, Roger


On 12/05/2018 01:12 PM, Mandy Chung wrote:



On 12/3/18 11:12 AM, Vicente Romero wrote:

Hi all,

Can I have the final nod to the JVM constants API, there have been 
some changes since the last review iteration. Thanks to the internal 
and external developers that have taken the time to provide feedback 
so far. The links to the last versions are:


webrev: http://cr.openjdk.java.net/~vromero/8210031/webrev.10/
javadoc: 
http://cr.openjdk.java.net/~vromero/8210031/javadoc.18/overview-summary.html
specdiff: 
http://cr.openjdk.java.net/~vromero/8210031/specdiff.08/overview-summary.html




I reviewed webrev.10 and overall looks good to me.  A couple of minor 
comments and some of them seems to be fixed already in amber repo.  
No need to generate a new webrev.


Nit: The javadoc of the new methods starts with "Returns", "Return", 
"Produce", "Create", "Resolve", "Compares" etc.  It'd be good to do a 
pass and fix the verb form consistently.


src/java.base/share/classes/java/lang/Class.java
   nit: Line 163 seems to have extra white-spaces, not aligned with 
the other superinterfaces.


src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java 
74 private static final Set suppressSubtypesSet

75 = Set.of("java.lang.Object",
76 "org.omg.CORBA.Object");

This is not related to your change. CORBA is no longer in JDK.
Maybe this special casing is no longer needed.  It may worth
filing a JBS issue to examine this.

ConstantDescs.java
  64 // Don't change the order of these declarations!

Is there any code depending on this order

Re: RFR: JDK-8210031: implementation for JVM Constants API

[Vicente Romero]

Hi Rogers,

Thanks for your comments, I have uploaded another webrev [1], some 
comments below:


[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.14/

On 12/6/18 11:07 AM, Roger Riggs wrote:

Hi Vicente,

On 12/05/2018 09:13 PM, Vicente Romero wrote:

Hi Roger,

Thanks for your comments, see my comments below. I have published 
another iteration at [1]


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8210031/webrev.12/




- bootStrapArgs() - the return is always non-null, with zero length 
array for non args?


it has to be, IMO, as the private constructor that is the only one 
that assigns a value to field bootstrapArgs makes sure that the 
assigned value is non-null
My point is that the javadoc is a specification; so the reader should 
not need to

look at an implementation to completely understand the required behavior.


got it, I added a comment to the specification to make it more clear





DynamicConstantDesc.java:
 - First sentences should end with period. "."

 - ofCanonical:  2nd sentence, "Classes ... produces ... a 
ConstantDesc"; grammar?


 - of Canaonical(): So a conforming implementation is not required 
to return the

    well known values, only suggested.

ClassDesc.java:
 - The create methods should not be required to create new 
instances.  Use "return"  instead of "create".


 - How are class descriptors created for 'nested' classes (as 
opposed to inner classes).

   It is worth describing or referencing how that should be done.


not sure I get what you mean. If you want to create a class 
descriptor for a nested class you should be able to invoke inner() 
with an string containing a number, what should be made more clear 
here in your opinion?
In other places in java specs, there is a distinction made between 
'inner' classes and 'nested' classes.
If the same API is used for both, it would be clearer to the reader if 
it mentioned nested.


the ::inner methods were renamed to ::nested



Thanks, Roger


Thanks
Vicente





MethodTypeDesc.java:
 - First sentences should end with period. "."

Regards, Roger


On 12/05/2018 01:12 PM, Mandy Chung wrote:



On 12/3/18 11:12 AM, Vicente Romero wrote:

Hi all,

Can I have the final nod to the JVM constants API, there have been 
some changes since the last review iteration. Thanks to the 
internal and external developers that have taken the time to 
provide feedback so far. The links to the last versions are:


webrev: http://cr.openjdk.java.net/~vromero/8210031/webrev.10/
javadoc: 
http://cr.openjdk.java.net/~vromero/8210031/javadoc.18/overview-summary.html
specdiff: 
http://cr.openjdk.java.net/~vromero/8210031/specdiff.08/overview-summary.html




I reviewed webrev.10 and overall looks good to me.  A couple of 
minor comments and some of them seems to be fixed already in amber 
repo.  No need to generate a new webrev.


Nit: The javadoc of the new methods starts with "Returns", 
"Return", "Produce", "Create", "Resolve", "Compares" etc. It'd be 
good to do a pass and fix the verb form consistently.


src/java.base/share/classes/java/lang/Class.java
   nit: Line 163 seems to have extra white-spaces, not aligned with 
the other superinterfaces.


src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/ClassWriterImpl.java 
74 private static final Set suppressSubtypesSet

75 = Set.of("java.lang.Object",
76 "org.omg.CORBA.Object");

This is not related to your change. CORBA is no longer in JDK.
Maybe this special casing is no longer needed.  It may worth
filing a JBS issue to examine this.

ConstantDescs.java
  64 // Don't change the order of these declarations!

Is there any code depending on this order?

DirectMethodHandleDesc.java
  46  * {@link MethodHandle}.  A {@linkplain 
DirectMethodHandleDescImpl} corresponds to


typo: DirectMethodHandleDescImpl

  57 /**
  58  * Kinds of method handles that can be described with 
{@linkplain DirectMethodHandleDesc}.

  59  */
  60 enum Kind {

This needs @since 12

DynamicCallSiteDesc.java
  59  * @param bootstrapMethod a {@link 
DirectMethodHandleDescImpl} describing the
  60  *    bootstrap method for the {@code 
invokedynamic}


typo: DirectMethodHandleDescImpl and in a few other @param

Mandy

Re: Type variable information is not always maintained for anonymous classes

[Vicente Romero]

Hi Sergey,

Thanks for your interest in the bug, yes if you feel like, please feel 
free to give it a try.


Vicente

On 12/7/18 6:53 PM, Sergey wrote:

Hi everyone,

Recently I've stumbled upon this bug
https://bugs.openjdk.java.net/browse/JDK-8213465
which is named the same way as in the header of an email. I've done a
little bit of
investigation and keen to fix it. Though I'm afraid that most likely fix
wouldn't be just
a one-liner. Thus I want to ask for a little bit of a guidance and make
sure, that I do not cross
anyone else. With that being said, if ticket isn't in progress and no one
minds I want to make
an attempt on it.

Thanks and regards,
Sergei

RFR JDK-8215300: additional changes to constants API

[Vicente Romero]

Hi,

Please review some final changes to the constants API. The changes 
should make the API clearer and more precise and were recommended in the 
CSR [3]


Thanks,
Vicente

[1] (jira issue) https://bugs.openjdk.java.net/browse/JDK-8215300
[2] (webrev) http://cr.openjdk.java.net/~vromero/8215300/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8202031

Re: RFR JDK-8215300: additional changes to constants API

[Vicente Romero]

Hi all,

I have provided another iteration to the webrev at [1]. This one 
includes additional changes to make sure that no array descriptor with 
more than 255 dimensions is created.


Thanks,
Vicente

http://cr.openjdk.java.net/~vromero/8215300/webrev.01/


On 12/12/18 12:02 PM, Vicente Romero wrote:

Hi,

Please review some final changes to the constants API. The changes 
should make the API clearer and more precise and were recommended in 
the CSR [3]


Thanks,
Vicente

[1] (jira issue) https://bugs.openjdk.java.net/browse/JDK-8215300
[2] (webrev) http://cr.openjdk.java.net/~vromero/8215300/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8202031

RFR: JDK-8215648: remove equals and hashCode implementations from j.l.i.VarHandle

[Vicente Romero]

Please review fix for [1] at [2]. The fx is basically removing the 
implementation of methods VarHandle::equals and VarHandle::hashCode to 
make var handles consistent with method handles which don't override the 
default implementation for these methods,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8215648
[2] http://cr.openjdk.java.net/~vromero/8215648/webrev.00/

Re: RFR: JDK-8215648: remove equals and hashCode implementations from j.l.i.VarHandle

[Vicente Romero]

Please review also the related CSR: 
https://bugs.openjdk.java.net/browse/JDK-8215649


Thanks,
Vicente

On 12/19/18 4:27 PM, Vicente Romero wrote:
Please review fix for [1] at [2]. The fx is basically removing the 
implementation of methods VarHandle::equals and VarHandle::hashCode to 
make var handles consistent with method handles which don't override 
the default implementation for these methods,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8215648
[2] http://cr.openjdk.java.net/~vromero/8215648/webrev.00/

Re: RFR - CSR JDK-8215490 Remove String::align

[Vicente Romero]

looks good,
Vicente

On 12/19/18 2:47 PM, Jim Laskey wrote:

CSR: https://bugs.openjdk.java.net/browse/JDK-8215490 


Thank you.

Cheers,

— Jim


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

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

RFR: JDK-8215510: j.l.c.ClassDesc is accepting descriptors not allowed by the spec

[Vicente Romero]

Please review the fix for bug [1] at [2]. Basically the constants API 
introduced as part of JEP-334 [3] were accepting descriptors and class 
names not allowed by the spec. This implementation fixes several issues 
found by TCK tests on JEP-334,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8215510
[2] http://cr.openjdk.java.net/~vromero/8215510/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8203252

Re: RFR: JDK-8215510: j.l.c.ClassDesc is accepting descriptors not allowed by the spec

[Vicente Romero]

I have updated the webrev after additional feedback from the TCK tester 
please check last version at [1]


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8215510/webrev.01


On 1/3/19 12:21 PM, Vicente Romero wrote:
Please review the fix for bug [1] at [2]. Basically the constants API 
introduced as part of JEP-334 [3] were accepting descriptors and class 
names not allowed by the spec. This implementation fixes several 
issues found by TCK tests on JEP-334,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8215510
[2] http://cr.openjdk.java.net/~vromero/8215510/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8203252

Re: Review Request JDK-8218461: test/jdk/java/lang/invoke/VarHandles should be generated rather than manually edited

[Vicente Romero]

looks good to me,
Vicente

On 2/6/19 3:13 PM, Mandy Chung wrote:

This patch fixes up X-VarHandleTest* template files that are
used to generate VarHandles tests.  Since VarHandle::equals
and VarHandle::hashCode methods are no longer implemented [1],
I rename the testEqualsAndHashCode method to testEquals
and further clean up the test case.

Webrev:
http://cr.openjdk.java.net/~mchung/jdk13/webrevs/8218461/webrev.00/

Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8215648

RFR: JDK-8219480: j.l.c.ClassDesc::arrayType(int rank) throws IllegalArgumentException if rank = 0

[Vicente Romero]

Please review the simple patch to fix [1] at [2]. The patch is simply 
adding a comment to the API, (javadoc) to sync it with the implementation.


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8219480
[2] http://cr.openjdk.java.net/~vromero/8219480/webrev.00/

Re: RFR: JDK-8219480: j.l.c.ClassDesc::arrayType(int rank) throws IllegalArgumentException if rank = 0

[Vicente Romero]

Hi Mandy,

Thanks for the review. I have uploaded a new iteration [1], please also 
review the CSR at [2]


Vicente

[1] http://cr.openjdk.java.net/~vromero/8219480/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8219587


On 2/21/19 5:22 PM, Mandy Chung wrote:



On 2/20/19 3:10 PM, Vicente Romero wrote:
Please review the simple patch to fix [1] at [2]. The patch is simply 
adding a comment to the API, (javadoc) to sync it with the 
implementation.


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8219480
[2] http://cr.openjdk.java.net/~vromero/8219480/webrev.00/


+ * @throws IllegalArgumentException if the rank is less or equal 
to zero or if the rank of the resulting array type is


typo: s/less or equal to/less than or equal to/

This needs a CSR for the spec fix.

Mandy

Re: RFR: JDK-8219480: j.l.c.ClassDesc::arrayType(int rank) throws IllegalArgumentException if rank = 0

[Vicente Romero]

thanks :)
Vicente

On 2/22/19 2:50 PM, Mandy Chung wrote:



On 2/22/19 8:37 AM, Vicente Romero wrote:

Hi Mandy,

Thanks for the review. I have uploaded a new iteration [1], please 
also review the CSR at [2]


Vicente

[1] http://cr.openjdk.java.net/~vromero/8219480/webrev.01/


Looks good.


[2] https://bugs.openjdk.java.net/browse/JDK-8219587


Reviewed.  I set the scope and interface kind of CSR for you.

Mandy

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Vicente Romero]

Hi Mandy,

The patch for nestmates [1] could be used as a reference. There a new 
method was added to class `com.sun.tools.javac.jvm.Target`, named: 
`hasNestmateAccess` which checks if a target is ready for nestmates or 
not. I think that you can follow a similar approach here.


Thanks,
Vicente

[1] http://hg.openjdk.java.net/jdk/jdk/rev/2f2af62dfac7

On 3/27/20 12:29 PM, Mandy Chung wrote:

Hi Jan,

Good point.  The javac change only applies to JDK 15 and later and the 
lambda proxy class is not a nestmate when running on JDK 14 or earlier.


I probably need the help from langtools team to fix this.  I'll give 
it a try.


Mandy

On 3/27/20 4:31 AM, Jan Lahoda wrote:

Hi Mandy,

Regarding the javac changes - should those be switched on/off 
depending the Target? Or, if one compiles with e.g. --release 14, 
will the newly generated output still work on JDK 14?


Jan

On 27. 03. 20 0:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The 
main changes are in core-libs and hotspot runtime area.  Small 
changes are made in javac, VM compiler (intrinsification of 
Class::isHiddenClass), JFR, JDI, and jcmd.  CSR [1]has been reviewed 
and is in the finalized state (see specdiff and javadoc below for 
reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03 



Hidden class is created via `Lookup::defineHiddenClass`. From JVM's 
point

of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not 
registered in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas `GetClassSignature` 
returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection. setAccessible(true) can 
still be called on reflected objects representing final fields in a 
hidden class and its access check will be suppressed but only have 
read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

    can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden 
class.
4. Field::setXXX method will throw IAE on a final field of a hidden 
class

    regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

    and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
    that holds the classes strongly referenced by its defining 
loader. There

    can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. Access 
control
    check no longer throws LinkageError but instead it will throw 
IAE with
    a clear message if a class fails to resolve/validate the nest 
host declared

    in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
    and generate a bridge method to desuger a method reference to a 
protected

    method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, and 
LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError 
and intends
to have the newly created class linked.  However, the implementation 
in 14

does not link the class.  A separate CSR [2] proposes to update the
implementation to match the spec.  This patch fixes the implementation.

The spec update on JVM TI, JDI and Instrumentation will be done as
a separate RFE [3].  This patch includes new tests for JVM TI and
java.instrument that validates how the existing APIs work for hidden 
classes.


javadoc/specdiff
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/api/
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/specdiff/ 



JVMS 5.4.4 change:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/Draft-JVMS-HiddenClasses.pdf 



CSR:
https://bugs.openjdk.java.net/browse/JDK-8238359

Thanks
Mandy
[1] https://bugs.openjdk.java.net/browse/JDK-8238359
[2] https://bugs.openjdk.java.net/browse/JDK-8240338
[3] https://bugs.openjdk.java.net/browse/JDK-8230502

Re: Review Request: 8238358: Implementation of JEP 371: Hidden Classes

[Vicente Romero]

Hi Mandy,

On 3/27/20 6:29 PM, Mandy Chung wrote:

Hi Vicente,

hasNestmateAccess is about VM supports static nestmates on JDK release 
>= 11.


I was not suggesting the use of `hasNestmateAccess` but to follow the 
same approach which is adding a new method at class `Target` to check if 
the new goodies were in the given target


However this is about javac --release 14 and the compiled classes may 
run on JDK 14 that lambda and string concat spin classes that are not 
nestmates. I have a patch with Jan's help:


http://cr.openjdk.java.net/~mchung/valhalla/webrevs/8171335/webrev-javac-target-release-14/index.html


which is what the patch above is doing



(you can apply the above patch on valhalla repo "nestmates" branch)

About testing, I wanted to run BridgeMethodsForLambdaTest and 
TestLambdaBytecode test with --release 14 but it turns out not 
straight-forward.  Any help would be appreciated.


thanks
Mandy


Vicente


On 3/27/20 2:15 PM, Vicente Romero wrote:

Hi Mandy,

The patch for nestmates [1] could be used as a reference. There a new 
method was added to class `com.sun.tools.javac.jvm.Target`, named: 
`hasNestmateAccess` which checks if a target is ready for nestmates 
or not. I think that you can follow a similar approach here.


Thanks,
Vicente

[1] http://hg.openjdk.java.net/jdk/jdk/rev/2f2af62dfac7

On 3/27/20 12:29 PM, Mandy Chung wrote:

Hi Jan,

Good point.  The javac change only applies to JDK 15 and later and 
the lambda proxy class is not a nestmate when running on JDK 14 or 
earlier.


I probably need the help from langtools team to fix this. I'll give 
it a try.


Mandy

On 3/27/20 4:31 AM, Jan Lahoda wrote:

Hi Mandy,

Regarding the javac changes - should those be switched on/off 
depending the Target? Or, if one compiles with e.g. --release 14, 
will the newly generated output still work on JDK 14?


Jan

On 27. 03. 20 0:57, Mandy Chung wrote:
Please review the implementation of JEP 371: Hidden Classes. The 
main changes are in core-libs and hotspot runtime area.  Small 
changes are made in javac, VM compiler (intrinsification of 
Class::isHiddenClass), JFR, JDI, and jcmd.  CSR [1]has been 
reviewed and is in the finalized state (see specdiff and javadoc 
below for reference).


Webrev:
http://cr.openjdk.java.net/~mchung/valhalla/webrevs/hidden-classes/webrev.03 



Hidden class is created via `Lookup::defineHiddenClass`. From 
JVM's point

of view, a hidden class is a normal class except the following:

- A hidden class has no initiating class loader and is not 
registered in any dictionary.
- A hidden class has a name containing an illegal character 
`Class::getName` returns `p.Foo/0x1234` whereas 
`GetClassSignature` returns "Lp/Foo.0x1234;".
- A hidden class is not modifiable, i.e. cannot be redefined or 
retransformed. JVM TI IsModifableClass returns false on a hidden.
- Final fields in a hidden class is "final".  The value of final 
fields cannot be overriden via reflection. setAccessible(true) can 
still be called on reflected objects representing final fields in 
a hidden class and its access check will be suppressed but only 
have read-access (i.e. can do Field::getXXX but not setXXX).


Brief summary of this patch:

1. A new Lookup::defineHiddenClass method is the API to create a 
hidden class.
2. A new Lookup.ClassOption enum class defines NESTMATE and STRONG 
option that

    can be specified when creating a hidden class.
3. A new Class::isHiddenClass method tests if a class is a hidden 
class.
4. Field::setXXX method will throw IAE on a final field of a 
hidden class

    regardless of the value of the accessible flag.
5. JVM_LookupDefineClass is the new JVM entry point for 
Lookup::defineClass

    and defineHiddenClass to create a class from the given bytes.
6. ClassLoaderData implementation is not changed.  There is one 
primary CLD
    that holds the classes strongly referenced by its defining 
loader. There

    can be zero or more additional CLDs - one per weak class.
7. Nest host determination is updated per revised JVMS 5.4.4. 
Access control
    check no longer throws LinkageError but instead it will throw 
IAE with
    a clear message if a class fails to resolve/validate the nest 
host declared

    in NestHost/NestMembers attribute.
8. JFR, jcmd, JDI are updated to support hidden classes.
9. update javac LambdaToMethod as lambda proxy starts using nestmates
    and generate a bridge method to desuger a method reference to 
a protected

    method in its supertype in a different package

This patch also updates StringConcatFactory, LambdaMetaFactory, 
and LambdaForms
to use hidden classes.  The webrev includes changes in nashorn to 
hidden class

and I will update the webrev if JEP 372 removes it any time soon.

We uncovered a bug in Lookup::defineClass spec throws LinkageError 
and intends
to have the newly created class linked.  However, the 
implementation in 14

does not link the class.  A separate 

RFR: JDK-8241627: Updating ASM to 8.0 for JDK 15

[Vicente Romero]

Hi all,

Please review this patch that updates the ASM version to appear in JDK15 
(from 7.0 to 8.0). Version 8.0 is compatible with JDK14 plus it has 
several experimental APIs to cover records and sealed types. We have 
also updated a number of tests (mostly record oriented tests) to use the 
updated APIs. Thanks to Igor Ignatyev for his help in particular for 
finding the source and fixing an issue that was provoking a failure at test:


open/test/jdk/jdk/jfr/jvm/TestLogOutput.java

This is the only substantial change that makes our copy of ASM different 
from the original, apart from several @SuppressWarnings that were added 
for the build to pass. The patch from Igor is this:


diff -r b2ca6a37c16b 
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java
--- 
a/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 07:04:27 2020 -0700
+++ 
b/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 17:14:59 2020 -0700

@@ -222,15 +222,15 @@
 }

 static boolean isWhitelisted(final String internalName) {
-    if (!internalName.startsWith("org/objectweb/asm/")) {
+    if (!internalName.startsWith("jdk/internal/org/objectweb/asm/")) {
 return false;
 }
 String member = 
"(Annotation|Class|Field|Method|Module|RecordComponent|Signature)";

 return internalName.contains("Test$")
 || Pattern.matches(
-    "org/objectweb/asm/util/Trace" + member + 
"Visitor(\\$.*)?", internalName)
+ "jdk/internal/org/objectweb/asm/util/Trace" + member + 
"Visitor(\\$.*)?", internalName)

 || Pattern.matches(
-    "org/objectweb/asm/util/Check" + member + 
"Adapter(\\$.*)?", internalName);
+ "jdk/internal/org/objectweb/asm/util/Check" + member + 
"Adapter(\\$.*)?", internalName);

 }

 static void checkIsPreview(final InputStream classInputStream) {


And it is basically adapting our copy to the different package prefix we 
use compared to the original ASM code. I have prepared two webrevs one 
at [1] which has the full change and one at [2] for which I have removed 
a lot of javadoc from the full webrev for reviewers that prefer less 
javadoc.


Thanks,
Vicente

PS, thanks to Remi for fixing in a very short time a bug I found in ASM 
related to the parsing of Record attributes


[1] http://cr.openjdk.java.net/~vromero/8241627/full/webrev.01/
[2] http://cr.openjdk.java.net/~vromero/8241627/no_javadoc/webrev.01/

Re: RFR: JDK-8241627: Updating ASM to 8.0 for JDK 15

[Vicente Romero]

Hi Paul,

On 4/14/20 2:05 PM, Paul Sandoz wrote:

Hi Vicente,

Looks ok from the changes required to integrate into the JDK.

Maybe the @SuppressWarnings warning annotations could be upstreamed?


do you mean to ask ASM team to include them? Sorry that I wasn't more 
specific, the warnings come from the use of experimental APIs (intended 
for JDK preview features), they have used to approach of "deprecating" 
the experimental APIs




Paul.


Vicente





On Apr 13, 2020, at 8:05 AM, Vicente Romero  wrote:

Hi all,

Please review this patch that updates the ASM version to appear in JDK15 (from 
7.0 to 8.0). Version 8.0 is compatible with JDK14 plus it has several 
experimental APIs to cover records and sealed types. We have also updated a 
number of tests (mostly record oriented tests) to use the updated APIs. Thanks 
to Igor Ignatyev for his help in particular for finding the source and fixing 
an issue that was provoking a failure at test:

open/test/jdk/jdk/jfr/jvm/TestLogOutput.java

This is the only substantial change that makes our copy of ASM different from 
the original, apart from several @SuppressWarnings that were added for the 
build to pass. The patch from Igor is this:

diff -r b2ca6a37c16b 
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java
--- a/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 07:04:27 2020 -0700
+++ b/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 17:14:59 2020 -0700
@@ -222,15 +222,15 @@
  }

  static boolean isWhitelisted(final String internalName) {
-if (!internalName.startsWith("org/objectweb/asm/")) {
+if (!internalName.startsWith("jdk/internal/org/objectweb/asm/")) {
  return false;
  }
  String member = 
"(Annotation|Class|Field|Method|Module|RecordComponent|Signature)";
  return internalName.contains("Test$")
  || Pattern.matches(
-"org/objectweb/asm/util/Trace" + member + 
"Visitor(\\$.*)?", internalName)
+ "jdk/internal/org/objectweb/asm/util/Trace" + member + "Visitor(\\$.*)?", 
internalName)
  || Pattern.matches(
-"org/objectweb/asm/util/Check" + member + 
"Adapter(\\$.*)?", internalName);
+ "jdk/internal/org/objectweb/asm/util/Check" + member + "Adapter(\\$.*)?", 
internalName);
  }

  static void checkIsPreview(final InputStream classInputStream) {


And it is basically adapting our copy to the different package prefix we use 
compared to the original ASM code. I have prepared two webrevs one at [1] which 
has the full change and one at [2] for which I have removed a lot of javadoc 
from the full webrev for reviewers that prefer less javadoc.

Thanks,
Vicente

PS, thanks to Remi for fixing in a very short time a bug I found in ASM related 
to the parsing of Record attributes

[1] http://cr.openjdk.java.net/~vromero/8241627/full/webrev.01/
[2] http://cr.openjdk.java.net/~vromero/8241627/no_javadoc/webrev.01/

Re: RFR: JDK-8241627: Updating ASM to 8.0 for JDK 15

[Vicente Romero]

On 4/14/20 2:42 PM, Paul Sandoz wrote:



On Apr 14, 2020, at 11:22 AM, Vicente Romero  wrote:

Hi Paul,

On 4/14/20 2:05 PM, Paul Sandoz wrote:

Hi Vicente,

Looks ok from the changes required to integrate into the JDK.

Maybe the @SuppressWarnings warning annotations could be upstreamed?

do you mean to ask ASM team to include them?

Yes.



Sorry that I wasn't more specific, the warnings come from the use of experimental APIs 
(intended for JDK preview features), they have used to approach of 
"deprecating" the experimental APIs


Ah, I see now, I should have looked more closely.  I guess it’s a policy 
decision for the ASM maintainers, since ASM has to depend internally on some 
experimental components, say Opcodes.ASM9_EXPERIMENTAL.


that's correct


Paul.

Vicente

Re: RFR: JDK-8241627: Updating ASM to 8.0 for JDK 15

[Vicente Romero]

ping, any other comment? are we ready to push this change?

Vicente

On 4/13/20 11:05 AM, Vicente Romero wrote:

Hi all,

Please review this patch that updates the ASM version to appear in 
JDK15 (from 7.0 to 8.0). Version 8.0 is compatible with JDK14 plus it 
has several experimental APIs to cover records and sealed types. We 
have also updated a number of tests (mostly record oriented tests) to 
use the updated APIs. Thanks to Igor Ignatyev for his help in 
particular for finding the source and fixing an issue that was 
provoking a failure at test:


open/test/jdk/jdk/jfr/jvm/TestLogOutput.java

This is the only substantial change that makes our copy of ASM 
different from the original, apart from several @SuppressWarnings that 
were added for the build to pass. The patch from Igor is this:


diff -r b2ca6a37c16b 
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java
--- 
a/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 07:04:27 2020 -0700
+++ 
b/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 17:14:59 2020 -0700

@@ -222,15 +222,15 @@
 }

 static boolean isWhitelisted(final String internalName) {
-    if (!internalName.startsWith("org/objectweb/asm/")) {
+    if 
(!internalName.startsWith("jdk/internal/org/objectweb/asm/")) {

 return false;
 }
 String member = 
"(Annotation|Class|Field|Method|Module|RecordComponent|Signature)";

 return internalName.contains("Test$")
 || Pattern.matches(
-    "org/objectweb/asm/util/Trace" + member + 
"Visitor(\\$.*)?", internalName)
+ "jdk/internal/org/objectweb/asm/util/Trace" + member + 
"Visitor(\\$.*)?", internalName)

 || Pattern.matches(
-    "org/objectweb/asm/util/Check" + member + 
"Adapter(\\$.*)?", internalName);
+ "jdk/internal/org/objectweb/asm/util/Check" + member + 
"Adapter(\\$.*)?", internalName);

 }

 static void checkIsPreview(final InputStream classInputStream) {


And it is basically adapting our copy to the different package prefix 
we use compared to the original ASM code. I have prepared two webrevs 
one at [1] which has the full change and one at [2] for which I have 
removed a lot of javadoc from the full webrev for reviewers that 
prefer less javadoc.


Thanks,
Vicente

PS, thanks to Remi for fixing in a very short time a bug I found in 
ASM related to the parsing of Record attributes


[1] http://cr.openjdk.java.net/~vromero/8241627/full/webrev.01/
[2] http://cr.openjdk.java.net/~vromero/8241627/no_javadoc/webrev.01/

Re: RFR: JDK-8241627: Updating ASM to 8.0 for JDK 15

[Vicente Romero]

Hi Chris,

On 4/20/20 4:43 AM, Chris Hegarty wrote:

I skimmed over the webrev, the changes seem fine.


thanks for the review


Thanks for updating the record serialization tests that use ASM.


sure np,



-Chris.


Vicente





On 17 Apr 2020, at 21:28, Vicente Romero  wrote:

ping, any other comment? are we ready to push this change?

Vicente

On 4/13/20 11:05 AM, Vicente Romero wrote:

Hi all,

Please review this patch that updates the ASM version to appear in JDK15 (from 
7.0 to 8.0). Version 8.0 is compatible with JDK14 plus it has several 
experimental APIs to cover records and sealed types. We have also updated a 
number of tests (mostly record oriented tests) to use the updated APIs. Thanks 
to Igor Ignatyev for his help in particular for finding the source and fixing 
an issue that was provoking a failure at test:

open/test/jdk/jdk/jfr/jvm/TestLogOutput.java

This is the only substantial change that makes our copy of ASM different from 
the original, apart from several @SuppressWarnings that were added for the 
build to pass. The patch from Igor is this:

diff -r b2ca6a37c16b 
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java
--- a/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 07:04:27 2020 -0700
+++ b/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 17:14:59 2020 -0700
@@ -222,15 +222,15 @@
  }

  static boolean isWhitelisted(final String internalName) {
-if (!internalName.startsWith("org/objectweb/asm/")) {
+if (!internalName.startsWith("jdk/internal/org/objectweb/asm/")) {
  return false;
  }
  String member = 
"(Annotation|Class|Field|Method|Module|RecordComponent|Signature)";
  return internalName.contains("Test$")
  || Pattern.matches(
-"org/objectweb/asm/util/Trace" + member + 
"Visitor(\\$.*)?", internalName)
+ "jdk/internal/org/objectweb/asm/util/Trace" + member + "Visitor(\\$.*)?", 
internalName)
  || Pattern.matches(
-"org/objectweb/asm/util/Check" + member + 
"Adapter(\\$.*)?", internalName);
+ "jdk/internal/org/objectweb/asm/util/Check" + member + "Adapter(\\$.*)?", 
internalName);
  }

  static void checkIsPreview(final InputStream classInputStream) {


And it is basically adapting our copy to the different package prefix we use 
compared to the original ASM code. I have prepared two webrevs one at [1] which 
has the full change and one at [2] for which I have removed a lot of javadoc 
from the full webrev for reviewers that prefer less javadoc.

Thanks,
Vicente

PS, thanks to Remi for fixing in a very short time a bug I found in ASM related 
to the parsing of Record attributes

[1] http://cr.openjdk.java.net/~vromero/8241627/full/webrev.01/
[2] http://cr.openjdk.java.net/~vromero/8241627/no_javadoc/webrev.01/

Re: RFR: JDK-8241627: Updating ASM to 8.0 for JDK 15

[Vicente Romero]

Hi,

Thanks for the reviews. I have just pushed the patch, ASM 8.0.1 will be 
the new version we will be using starting from JDK 15.


Thanks,
Vicente

On 4/20/20 1:11 PM, Vicente Romero wrote:

Hi Chris,

On 4/20/20 4:43 AM, Chris Hegarty wrote:

I skimmed over the webrev, the changes seem fine.


thanks for the review


Thanks for updating the record serialization tests that use ASM.


sure np,



-Chris.


Vicente




On 17 Apr 2020, at 21:28, Vicente Romero  
wrote:


ping, any other comment? are we ready to push this change?

Vicente

On 4/13/20 11:05 AM, Vicente Romero wrote:

Hi all,

Please review this patch that updates the ASM version to appear in 
JDK15 (from 7.0 to 8.0). Version 8.0 is compatible with JDK14 plus 
it has several experimental APIs to cover records and sealed types. 
We have also updated a number of tests (mostly record oriented 
tests) to use the updated APIs. Thanks to Igor Ignatyev for his 
help in particular for finding the source and fixing an issue that 
was provoking a failure at test:


open/test/jdk/jdk/jfr/jvm/TestLogOutput.java

This is the only substantial change that makes our copy of ASM 
different from the original, apart from several @SuppressWarnings 
that were added for the build to pass. The patch from Igor is this:


diff -r b2ca6a37c16b 
src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java
--- 
a/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 07:04:27 2020 -0700
+++ 
b/src/java.base/share/classes/jdk/internal/org/objectweb/asm/Constants.java 
Fri Apr 10 17:14:59 2020 -0700

@@ -222,15 +222,15 @@
  }

  static boolean isWhitelisted(final String internalName) {
-    if (!internalName.startsWith("org/objectweb/asm/")) {
+    if 
(!internalName.startsWith("jdk/internal/org/objectweb/asm/")) {

  return false;
  }
  String member = 
"(Annotation|Class|Field|Method|Module|RecordComponent|Signature)";

  return internalName.contains("Test$")
  || Pattern.matches(
-    "org/objectweb/asm/util/Trace" + member + 
"Visitor(\\$.*)?", internalName)
+ "jdk/internal/org/objectweb/asm/util/Trace" + member + 
"Visitor(\\$.*)?", internalName)

  || Pattern.matches(
-    "org/objectweb/asm/util/Check" + member + 
"Adapter(\\$.*)?", internalName);
+ "jdk/internal/org/objectweb/asm/util/Check" + member + 
"Adapter(\\$.*)?", internalName);

  }

  static void checkIsPreview(final InputStream classInputStream) {


And it is basically adapting our copy to the different package 
prefix we use compared to the original ASM code. I have prepared 
two webrevs one at [1] which has the full change and one at [2] for 
which I have removed a lot of javadoc from the full webrev for 
reviewers that prefer less javadoc.


Thanks,
Vicente

PS, thanks to Remi for fixing in a very short time a bug I found in 
ASM related to the parsing of Record attributes


[1] http://cr.openjdk.java.net/~vromero/8241627/full/webrev.01/
[2] http://cr.openjdk.java.net/~vromero/8241627/no_javadoc/webrev.01/

RFR: JDK-8227046: compiler implementation for sealed classes, JDK-8227047: Javadoc for sealed types and JDK-8227044: javax.lang.model for sealed classes

[Vicente Romero]

Hi,

Please review this patch for the compiler, javadoc and javax.lang.model 
support for the JEP 360 Sealed Classes (Preview). The changes are 
provided at [1], which implements the latest JLS for sealed types [2]. 
The patch also include the needed changes to javadoc and 
javax.lang.model to support sealed types. The CSR witht the changes in 
the javax.lang.model spec is at [3]. The sealed types JEP is accessible 
at [4]. There is an ongoing review for the VM and core-libs code of 
sealed types [5] and that code hasn't been included in this webrev,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8227046/webrev.00/
[2] 
http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html 


[3] https://bugs.openjdk.java.net/browse/JDK-8244367
[4] https://openjdk.java.net/jeps/360
[5] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066440.html

Re: RFR: JDK-8225056 VM support for sealed classes

[Vicente Romero]

Hi David,


src/java.base/share/classes/java/lang/Class.java

There needs to be a CSR request for these changes.


yes there is one already: https://bugs.openjdk.java.net/browse/JDK-8244556


+  * Returns an array containing {@code ClassDesc} objects 
representing all the
+  * permitted subclasses of this {@linkplain Class} if it is 
sealed. Returns an empty array if this

+  * {@linkplain Class} is not sealed.

should add "or this class represents an array or primitive type" 
(using the standard wording for such cases).


well given that array and primitive classes are not sealed classes I 
think we are already covered by the method's spec.




+  * @throws IllegalArgumentException if a class descriptor is not 
in the correct format


IllegalArgumentException is not an appropriate exception to use as 
this method takes no arguments. If the class descriptor is not valid 
and it comes from the VM then I think we have a problem with how the 
VM validates class descriptors. Any IAE from ClassDesc.of should be 
caught and converted to a more suitable exception type - preferably 
InternalError if the VM should always return valid strings.


we agree with you here, this will be fixed in the next review iteration.



+ public ClassDesc[] getPermittedSubclasses() {

As mentioned for jvm.cpp this Java code should do the isArray() and 
isPrimitive() check before calling the VM.


agreed.




Thanks,
Vicente

Re: RFR: JDK-8227046: compiler implementation for sealed classes, JDK-8227047: Javadoc for sealed types and JDK-8227044: javax.lang.model for sealed classes

[Vicente Romero]

ass

test/langtools/tools/javac/diags/examples/PermitsCantListDeclaringClass.java:30: 
error: invalid permits clause

sealed class C permits C {}
   ^
  (must not include the declaring class: C)

Here I recommend something like:

(illegal self-reference in permits clause)

* PermitsCantListSuperType

test/langtools/tools/javac/diags/examples/PermitsCantListSuperType.java:32: 
error: invalid permits clause

sealed class C implements I permits I {}
    ^
  (must not include a supertype: I)

I suggest:

(illegal reference to supertype I)

* PermitsInNoSealedClass

test/langtools/tools/javac/diags/examples/PermitsInNoSealedClass.java:30: 
error: invalid permits clause

class C permits Sub {}
    ^
  (class must be sealed)

This is good, but I noted that if you change the test to use an 
interface, the message still says "class" - the kindname should be 
used here.


* SealedMustHaveSubtypes

test/langtools/tools/javac/diags/examples/SealedMustHaveSubtypes.java:29: 
error: sealed class must have subclasses

sealed class Sealed {}
   ^

I think this message reflects one of the main issues with the general 
type vs. class dichotomy. A subclass, in JLS lingo is e.g. `B` where 
`B extends A`. Interfaces do not play in the mix - they are not 
considered subclasses. The word subtypes could be more general - but 
again, it is a bit imprecise, since we're talking about declarations 
here, not types. I'll defer this conundrum to our spec gurus :-)



Cheers
Maurizio



On 18/05/2020 23:42, Vicente Romero wrote:

Hi,

Please review this patch for the compiler, javadoc and 
javax.lang.model support for the JEP 360 Sealed Classes (Preview). 
The changes are provided at [1], which implements the latest JLS for 
sealed types [2]. The patch also include the needed changes to 
javadoc and javax.lang.model to support sealed types. The CSR witht 
the changes in the javax.lang.model spec is at [3]. The sealed types 
JEP is accessible at [4]. There is an ongoing review for the VM and 
core-libs code of sealed types [5] and that code hasn't been included 
in this webrev,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8227046/webrev.00/
[2] 
http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html 


[3] https://bugs.openjdk.java.net/browse/JDK-8244367
[4] https://openjdk.java.net/jeps/360
[5] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066440.html

Re: RFR: JDK-8227046: compiler implementation for sealed classes, JDK-8227047: Javadoc for sealed types and JDK-8227044: javax.lang.model for sealed classes

[Vicente Romero]

Hi Maurizio,

Right good catch I forgot to add the MONKEY_AT to the list of expected 
tokens. I have fixed that. I have published another iteration at [1]. 
Apart from the MONKEY_AT issue addressed at the parser this iteration also:


 - adds another error key to compiler.properties, this new key is 
oriented to show a more explicit error message when an interface with no 
`non-sealed` or `sealed` modifier extends a sealed interface. A class in 
this position can also be `final` but this is not possible for interfaces.
 - changes to PrintingProcessor and to 
test/langtools/tools/javac/processing/model/element/TestSealed.java 
suggested by Joe in this thread
 - adds a new subtest at SealedCompilationTests testing the sealed and 
non-sealed modifiers being followed by different modifiers or annotations
 - modified a subtest also at SealedCompilationTests, testPrinting, 
that was failing on Windows


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8227046/webrev.03.delta/

On 5/25/20 6:37 AM, Maurizio Cimadamore wrote:

Hi,
changes look good, but the parser changes do not convince me. Now that 
the logic is more clearly defined, I see some issues e.g. there seems 
to be a tight coupling by where "sealed" or "non-sealed" is, and the 
fact that the following token must be e.g. "class". This is unlike 
other modifiers which can in fact appear in any configuation.


I believe that you can break the current logic by adding an extra 
annotation between the "sealed" token and the "class" token e.g.


sealed @foo class Bar { }

But maybe the quick fix would be to add MONKEY_AT to the set of 
expected tokens after sealed/non-sealed.


Maurizio


On 22/05/2020 19:25, Vicente Romero wrote:

Hi,

Thanks Jan and Maurizio for the reviews. I have created another 
iteration that I hope addresses all of the comments. I recognize that 
dealing with `sealed`, `non-sealed` is the most complicated part as 
there is no guide right now from the spec. So I have tried to make 
them contextual keywords, for some informal definition of the 
concept, I think that with more success for the `sealed` case. So 
going in more detail this iteration includes:


 - ClassTree::getPermitsClause now returns `List.of()`
 - Types::findDescriptorInternal has been modified to fail on sealed 
interfaces
 - several error messages have been updated as suggested, on this 
front given that when a class list the same interface twice in the 
implements clause, the second occurrence is the one flagged, I did 
the same for repeated names in the permits clause

 - modifications in ClassReader and ClassWriter as suggested
 - JavacParser, the bulk of the changes are concentrated here, as 
mentioned above the goal has been to try to implement the contextual 
keyword thing and also give a nice error message in several corner 
case situations detected in the reviews

 - more tests were added

Thanks,
Vicente

On 5/21/20 8:14 AM, Maurizio Cimadamore wrote:

Hi Vicente,
looks very good. Some comments below.

* the parser logic is clever in its use of position to apply 
context-dependent keyword detection; as Jan says, perhaps just share 
the code so that the position checks are not repeated.


* I found one very edge-case quirk in the context-dependent logic; 
not sure how we wanna address:


class Foo {
    sealed m() {}
}

This fails with:

Error: invalid method declaration; return type required

As javac parses non-sealed as a modifier, and then expects a type. I 
think this is probably reasonable, but it's not as context-dependent 
as it could be I guess.


* This case:

class Bar { }
sealed @interface Foo permits Bar

Fails as expected, but only because Bar doesn't extends Foo. I 
believe we'd like to ban sealed on annotations more eagerly. Same 
for non-sealed. For enums and records (which are non-extensible) the 
compiler does the right thing and tells me that I can't just use 
sealed/non-sealed there.


* The recovery logic in case preview features aren't enabled leaves 
something to be desired. For instance, if I compile this w/o 
--enable-preview:


 record Foo() {}

I get a very sensible error:

records are a preview feature and are disabled by default.
    (use --enable-preview to enable records)

However, if I compiler this w/o --enable-preview:

sealed class Foo {}

I get this:

error: class, interface, or enum expected

(no mention of preview features)

It gets worse if I also specify a `permits`.

* As Jan mentioned, type parameters on permitted types should be 
banned, not silently cleared in TypeEnter


* Overall the type enter logic seems robust - I've tried several 
examples swapping superclass/subclass - using references to nested 
classes in supertype declaration, and it all works. Well done.


* The error for lambda expressions leaves to be desired:

sealed interface Action {
 void doAction();
}

class Test {
  Action a = () -> { };
}

Re: RFR: JDK-8227046: compiler implementation for sealed classes, JDK-8227047: Javadoc for sealed types and JDK-8227044: javax.lang.model for sealed classes

[Vicente Romero]

thanks to all the reviewers, the code for sealed classes has already 
been pushed to mainline.


Vicente

On 5/18/20 6:42 PM, Vicente Romero wrote:

Hi,

Please review this patch for the compiler, javadoc and 
javax.lang.model support for the JEP 360 Sealed Classes (Preview). The 
changes are provided at [1], which implements the latest JLS for 
sealed types [2]. The patch also include the needed changes to javadoc 
and javax.lang.model to support sealed types. The CSR witht the 
changes in the javax.lang.model spec is at [3]. The sealed types JEP 
is accessible at [4]. There is an ongoing review for the VM and 
core-libs code of sealed types [5] and that code hasn't been included 
in this webrev,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8227046/webrev.00/
[2] 
http://cr.openjdk.java.net/~gbierman/jep360/jep360-20200513/specs/sealed-classes-jls.html 


[3] https://bugs.openjdk.java.net/browse/JDK-8244367
[4] https://openjdk.java.net/jeps/360
[5] 
https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-May/066440.html

RFR: JDK-8245958: j.l.Record need to mention that canonical constructor may not be public

[Vicente Romero]

Please review this API update. The API of java.lang.Record needs to be 
updated after the changes introduced by JEP 384: Records (Second 
Preview) [1], in particular this JEP changed the constraint that the 
canonical constructor had to be public. The associated bug is [2], the 
changeset is at [3] and the related CSR is at [4],


Thanks,
Vicente

[1] https://openjdk.java.net/jeps/384
[2] https://bugs.openjdk.java.net/browse/JDK-8245958
[3] http://cr.openjdk.java.net/~vromero/8245958/webrev.00/
[4] https://bugs.openjdk.java.net/browse/JDK-8246629

RFR: JDK-8246098: API for Class::permittedSubclasses should clarify if returned elements are ordered or not

[Vicente Romero]

Please review this small change to the API for 
java.lang.Class::permittedSubclasses which clarifies that no particular 
order of the returned elements should be assumed by clients. The related 
bug is at [1], the webrev at [2] and the associated CSR at [3],


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8246098
[2] http://cr.openjdk.java.net/~vromero/8246098/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8247361

Re: RFR: JDK-8246098: API for Class::permittedSubclasses should clarify if returned elements are ordered or not

[Vicente Romero]

thanks for the review,
Vicente

On 6/11/20 4:47 AM, Chris Hegarty wrote:



On 10 Jun 2020, at 23:23, Vicente Romero  wrote:

Please review this small change to the API for 
java.lang.Class::permittedSubclasses which clarifies that no particular order 
of the returned elements should be assumed by clients. The related bug is at 
[1], the webrev at [2] and the associated CSR at [3],

LGTM. I added myself as reviewer on the CSR.

-Chris.


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8246098
[2] http://cr.openjdk.java.net/~vromero/8246098/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8247361

RFR: JDK-8246804: Incorrect copyright header in TypeAnnotationParser.java

[Vicente Romero]

Please review this very simple fix for [1] at [2]. The fix is a very 
simple one liner change,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8246804
[2] http://cr.openjdk.java.net/~vromero/8246804/webrev.00/

RFR: 8246774: Record Classes (final) implementation

[Vicente Romero]

Co-authored-by: Vicente Romero 
Co-authored-by: Harold Seigel 
Co-authored-by: Jonathan Gibbons 
Co-authored-by: Brian Goetz 
Co-authored-by: Maurizio Cimadamore 
Co-authored-by: Joe Darcy 
Co-authored-by: Chris Hegarty 
Co-authored-by: Jan Lahoda 

-

Commit messages:
 - 8246774: Record Classes (final) implementation

Changes: https://git.openjdk.java.net/jdk/pull/290/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8246774
  Stats: 495 lines in 95 files changed: 23 ins; 362 del; 110 mod
  Patch: https://git.openjdk.java.net/jdk/pull/290.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation

[Vicente Romero]

On Mon, 21 Sep 2020 21:30:51 GMT, Vicente Romero  wrote:

> Co-authored-by: Vicente Romero 
> Co-authored-by: Harold Seigel 
> Co-authored-by: Jonathan Gibbons 
> Co-authored-by: Brian Goetz 
> Co-authored-by: Maurizio Cimadamore 
> Co-authored-by: Joe Darcy 
> Co-authored-by: Chris Hegarty 
> Co-authored-by: Jan Lahoda 

Please review the fix for [1]. The intention of this patch is to make records 
final removing the need to
use --enable-preview in order to be able to include a record declaration in a 
source or for the VM to execute code
compiled from record classes,

Thanks

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

-

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation

[Vicente Romero]

On Mon, 21 Sep 2020 21:53:05 GMT, Joe Darcy  wrote:

>> Please review the fix for [1]. The intention of this patch is to make 
>> records final removing the need to
>> use --enable-preview in order to be able to include a record declaration in 
>> a source or for the VM to execute code
>> compiled from record classes,  Thanks
>> 
>> [1] https://bugs.openjdk.java.net/browse/JDK-8246774
>
> Hi Vicente,
> Please file a separate subtask for the javax.lang.model changes. This helps 
> with the JSR 269 MR paperwork.
> Thanks,
> -Joe

note: I have removed from the original patch the code related to 
javax.lang.model, I will publish them in a separate PR

-

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation [v2]

[Vicente Romero]

> Co-authored-by: Vicente Romero 
> Co-authored-by: Harold Seigel 
> Co-authored-by: Jonathan Gibbons 
> Co-authored-by: Brian Goetz 
> Co-authored-by: Maurizio Cimadamore 
> Co-authored-by: Joe Darcy 
> Co-authored-by: Chris Hegarty 
> Co-authored-by: Jan Lahoda 

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  removing the javax.lang.model related code to be moved to a separate bug

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/290/files
  - new: https://git.openjdk.java.net/jdk/pull/290/files/9eedb3ab..543e5054

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=00-01

  Stats: 134 lines in 12 files changed: 130 ins; 0 del; 4 mod
  Patch: https://git.openjdk.java.net/jdk/pull/290.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation

[Vicente Romero]

good catch Chris, thanks for the patch,

Vicente

On 9/22/20 5:51 AM, Chris Hegarty wrote:

On Mon, 21 Sep 2020 23:21:18 GMT, Vicente Romero  wrote:


Hi Vicente,
Please file a separate subtask for the javax.lang.model changes. This helps 
with the JSR 269 MR paperwork.
Thanks,
-Joe

note: I have removed from the original patch the code related to 
javax.lang.model, I will publish them in a separate PR

@vicente-romero-oracle I noticed that we can also remove the preview args from 
the record serialization tests and
ObjectMethodsTest. I opened a PR against the branch in your fork. You should be 
able to just merge in the changes. See
https://github.com/vicente-romero-oracle/jdk/pull/1

-

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation [v3]

[Vicente Romero]

> Co-authored-by: Vicente Romero 
> Co-authored-by: Harold Seigel 
> Co-authored-by: Jonathan Gibbons 
> Co-authored-by: Brian Goetz 
> Co-authored-by: Maurizio Cimadamore 
> Co-authored-by: Joe Darcy 
> Co-authored-by: Chris Hegarty 
> Co-authored-by: Jan Lahoda 

Vicente Romero has updated the pull request incrementally with three additional 
commits since the last revision:

 - Merge pull request #1 from ChrisHegarty/record-serial-tests
   
   Remove preview args from JDK tests
 - Remove preview args from ObjectMethodsTest
 - Remove preview args from record serialization tests

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/290/files
  - new: https://git.openjdk.java.net/jdk/pull/290/files/543e5054..26b80775

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=01-02

  Stats: 95 lines in 21 files changed: 0 ins; 35 del; 60 mod
  Patch: https://git.openjdk.java.net/jdk/pull/290.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation [v4]

[Vicente Romero]

> Co-authored-by: Vicente Romero 
> Co-authored-by: Harold Seigel 
> Co-authored-by: Jonathan Gibbons 
> Co-authored-by: Brian Goetz 
> Co-authored-by: Maurizio Cimadamore 
> Co-authored-by: Joe Darcy 
> Co-authored-by: Chris Hegarty 
> Co-authored-by: Jan Lahoda 

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  modifiying @since from 14 to 16

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/290/files
  - new: https://git.openjdk.java.net/jdk/pull/290/files/26b80775..514b0a80

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=02-03

  Stats: 8 lines in 7 files changed: 0 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/290.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation [v3]

[Vicente Romero]

On Thu, 24 Sep 2020 12:23:13 GMT, Coleen Phillimore  wrote:

>> Vicente Romero has updated the pull request incrementally with three 
>> additional commits since the last revision:
>> 
>>  - Merge pull request #1 from ChrisHegarty/record-serial-tests
>>
>>Remove preview args from JDK tests
>>  - Remove preview args from ObjectMethodsTest
>>  - Remove preview args from record serialization tests
>
> The classfile parser changes look good to me.

I have modified the `@since`: 14 -> 16

-

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation [v5]

[Vicente Romero]

> Co-authored-by: Vicente Romero 
> Co-authored-by: Harold Seigel 
> Co-authored-by: Jonathan Gibbons 
> Co-authored-by: Brian Goetz 
> Co-authored-by: Maurizio Cimadamore 
> Co-authored-by: Joe Darcy 
> Co-authored-by: Chris Hegarty 
> Co-authored-by: Jan Lahoda 

Vicente Romero has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now
contains seven commits:

 - Merge branch 'master' into JDK-8246774
 - modifiying @since from 14 to 16
 - Merge pull request #1 from ChrisHegarty/record-serial-tests
   
   Remove preview args from JDK tests
 - Remove preview args from ObjectMethodsTest
 - Remove preview args from record serialization tests
 - removing the javax.lang.model related code to be moved to a separate bug
 - 8246774: Record Classes (final) implementation
   
   Co-authored-by: Vicente Romero 
   Co-authored-by: Harold Seigel 
   Co-authored-by: Jonathan Gibbons 
   Co-authored-by: Brian Goetz 
   Co-authored-by: Maurizio Cimadamore 
   Co-authored-by: Joe Darcy 
   Co-authored-by: Chris Hegarty 
   Co-authored-by: Jan Lahoda 

-

Changes: https://git.openjdk.java.net/jdk/pull/290/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=04
  Stats: 464 lines in 104 files changed: 23 ins; 267 del; 174 mod
  Patch: https://git.openjdk.java.net/jdk/pull/290.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation [v6]

[Vicente Romero]

> Co-authored-by: Vicente Romero 
> Co-authored-by: Harold Seigel 
> Co-authored-by: Jonathan Gibbons 
> Co-authored-by: Brian Goetz 
> Co-authored-by: Maurizio Cimadamore 
> Co-authored-by: Joe Darcy 
> Co-authored-by: Chris Hegarty 
> Co-authored-by: Jan Lahoda 

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  adding missing changes to some tests

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/290/files
  - new: https://git.openjdk.java.net/jdk/pull/290/files/89f7cc54..915b67e0

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=05
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=04-05

  Stats: 71 lines in 5 files changed: 9 ins; 11 del; 51 mod
  Patch: https://git.openjdk.java.net/jdk/pull/290.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: Record Classes (final) implementation [v3]

[Vicente Romero]

On Thu, 24 Sep 2020 15:45:22 GMT, Vicente Romero  wrote:

>> The classfile parser changes look good to me.
>
> I have modified the `@since`: 14 -> 16

[CSR: Record Classes](https://bugs.openjdk.java.net/browse/JDK-8253605)

-

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: implement Record Classes as a standard feature in Java [v12]

[Vicente Romero]

> 8246774: implement Record Classes as a standard feature in Java

Vicente Romero has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now
contains 10 commits:

 - Merge branch 'master' into JDK-8246774
 - Merge branch 'master' into JDK-8246774
 - removing unused jcod file
 - remove unnecessary jtreg tags from tests, remove othervm etc when applicable
 - adding missing changes to some tests
 - modifiying @since from 14 to 16
 - Remove preview args from ObjectMethodsTest
 - Remove preview args from record serialization tests
 - removing the javax.lang.model related code to be moved to a separate bug
 - 8246774: Record Classes (final) implementation
   
   Co-authored-by: Vicente Romero 
   Co-authored-by: Harold Seigel 
   Co-authored-by: Jonathan Gibbons 
   Co-authored-by: Brian Goetz 
   Co-authored-by: Maurizio Cimadamore 
   Co-authored-by: Joe Darcy 
   Co-authored-by: Chris Hegarty 
   Co-authored-by: Jan Lahoda 

-

Changes: https://git.openjdk.java.net/jdk/pull/290/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=11
  Stats: 856 lines in 109 files changed: 72 ins; 562 del; 222 mod
  Patch: https://git.openjdk.java.net/jdk/pull/290.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290

PR: https://git.openjdk.java.net/jdk/pull/290

Re: RFR: 8246774: implement Record Classes as a standard feature in Java [v13]

[Vicente Romero]

> 8246774: implement Record Classes as a standard feature in Java

Vicente Romero has updated the pull request incrementally with one additional 
commit since the last revision:

  removing reference to unused jcod file from test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/290/files
  - new: https://git.openjdk.java.net/jdk/pull/290/files/5bfbde59..3e472bc3

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=12
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=290&range=11-12

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/290.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/290/head:pull/290

PR: https://git.openjdk.java.net/jdk/pull/290

Integrated: 8246774: implement Record Classes as a standard feature in Java

[Vicente Romero]

On Mon, 21 Sep 2020 21:30:51 GMT, Vicente Romero  wrote:

> 8246774: implement Record Classes as a standard feature in Java

This pull request has now been integrated.

Changeset: c17d5851
Author:    Vicente Romero 
URL:   https://git.openjdk.java.net/jdk/commit/c17d5851
Stats: 856 lines in 109 files changed: 72 ins; 562 del; 222 mod

8246774: implement Record Classes as a standard feature in Java

Co-authored-by: Vicente Romero 
Co-authored-by: Harold Seigel 
Co-authored-by: Chris Hegarty 
Reviewed-by: coleenp, jlahoda, sspitsyn, chegar

-

PR: https://git.openjdk.java.net/jdk/pull/290

Integrated: 8253455: Record Classes javax.lang.model changes

[Vicente Romero]

On Mon, 21 Sep 2020 23:31:51 GMT, Vicente Romero  wrote:

> Please review the fix for 
> [JDK-8253455](https://bugs.openjdk.java.net/browse/JDK-8253455) which is part 
> of the effort
> to make records a final feature of the Java Language. The rest of the code 
> has been published in
> [PR#290](https://github.com/openjdk/jdk/pull/290)  Thanks,
> Vicente

This pull request has now been integrated.

Changeset: 272bb5d5
Author:Vicente Romero 
URL:   https://git.openjdk.java.net/jdk/commit/272bb5d5
Stats: 144 lines in 12 files changed: 5 ins; 126 del; 13 mod

8253455: Record Classes javax.lang.model changes

Reviewed-by: darcy

-

PR: https://git.openjdk.java.net/jdk/pull/291

Re: RFR: 8250625: Compiler implementation of Pattern Matching for instanceof (Final)

[Vicente Romero]

On Thu, 8 Oct 2020 11:49:17 GMT, Jan Lahoda  wrote:

> This is the current proposed patch for the upcoming JEP 394, for pattern 
> matching for instanceof.
> 
> A summary of changes:
> -making the feature permanent (non-preview)
> -making the binding variables non-final (as per current specification 
> proposal)
> -producing a compile-time error for the case where the expression's type is a 
> subtype of the type test pattern's type
>  (as per current specification proposal)
> -changing the AST structure so that the binding variable has a VariableTree 
> in the AST. BindingPatternTree is preserved
>  and encloses the VariableTree. The reason is better consistency in the API, 
> with nodes like CatchTree, EnhancedForLoop
>  Tree, etc.
> 
> This change will not be integrated until JEP 394 is targetted.

Changes requested by vromero (Reviewer).

test/langtools/tools/javac/patterns/BindingsTest1.java line 28:

> 26:  * @bug 8231827
> 27:  * @summary Basic tests for bindings from instanceof
> 28:  * @compile BindingsTest1.java

the @compile can be removed

test/langtools/tools/javac/patterns/BindingsTest1.java line 29:

> 27:  * @summary Basic tests for bindings from instanceof
> 28:  * @compile BindingsTest1.java
> 29:  * @run main BindingsTest1

and this line too

test/langtools/tools/javac/patterns/ExamplesFromProposal.java line 29:

> 27:  * @summary All example code from "Pattern Matching for Java" document, 
> released April 2017, adjusted to current
> state (no switches, etc) 28:  * @compile ExamplesFromProposal.java
> 29:  * @run main ExamplesFromProposal

these two lines can be removed now, there are other tests in this commit in 
which they can also be eliminated

test/langtools/tools/javac/patterns/LocalVariableTable.java line 29:

> 27:  * @summary Ensure the LV table entries are generated for bindings
> 28:  * @modules jdk.jdeps/com.sun.tools.classfile
> 29:  * @compile -g LocalVariableTable.java

I believe all tests are always compiled with `-g` option set, not related to 
your patch but we could probably remove
that line as superfluous

test/langtools/tools/javac/patterns/PatternsSimpleVisitorTest.java line 108:

> 106: StringWriter out = new StringWriter();
> 107: JavacTask ct = (JavacTask) tool.getTask(out, null, noErrors,
> 108: List.of(), null,

nit: what about passing `null` instead of `List.of()`?

-

PR: https://git.openjdk.java.net/jdk/pull/559

Re: RFR: 8250625: Compiler implementation of Pattern Matching for instanceof (Final) [v2]

[Vicente Romero]

On Tue, 20 Oct 2020 12:03:39 GMT, Jan Lahoda  wrote:

>> This is the current proposed patch for the upcoming JEP 394, for pattern 
>> matching for instanceof.
>> 
>> A summary of changes:
>> -making the feature permanent (non-preview)
>> -making the binding variables non-final (as per current specification 
>> proposal)
>> -producing a compile-time error for the case where the expression's type is 
>> a subtype of the type test pattern's type
>>  (as per current specification proposal)
>> -changing the AST structure so that the binding variable has a VariableTree 
>> in the AST. BindingPatternTree is preserved
>>  and encloses the VariableTree. The reason is better consistency in the API, 
>> with nodes like CatchTree, EnhancedForLoop
>>  Tree, etc.
>> 
>> This change will not be integrated until JEP 394 is targetted.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev
> excludes the unrelated changes brought in by the merge/rebase. The pull 
> request contains 15 additional commits since
> the last revision:
>  - Cleanup: using a null instead of List.of() as a parameter to 
> JavaCompiler.getTask
>  - Merge branch 'master' into patterns-instanceof3
>  - Fixing more tests.
>  - Correcting positions.
>  - Improve the AST model.
>  - Merge branch 'master' into patterns-instanceof3
>  - Updating @since tags.
>  - Merge branch 'master' into patterns-instanceof3
>  - Cleaning up preview comments in javadoc.
>  - Merge branch 'master' into patterns-instanceof3
>  - ... and 5 more: 
> https://git.openjdk.java.net/jdk/compare/3b3bcc2a...5978bca0

src/jdk.compiler/share/classes/com/sun/source/tree/BindingPatternTree.java line 
48:

> 46:  * @deprecated Use getVariable().getType()
> 47:  */
> 48: @Deprecated

shouldn't we use `@Deprecated(since=versionNumber)`

-

PR: https://git.openjdk.java.net/jdk/pull/559

Re: RFR: 8250625: Compiler implementation of Pattern Matching for instanceof (Final) [v2]

[Vicente Romero]

On Tue, 20 Oct 2020 12:03:39 GMT, Jan Lahoda  wrote:

>> This is the current proposed patch for the upcoming JEP 394, for pattern 
>> matching for instanceof.
>> 
>> A summary of changes:
>> -making the feature permanent (non-preview)
>> -making the binding variables non-final (as per current specification 
>> proposal)
>> -producing a compile-time error for the case where the expression's type is 
>> a subtype of the type test pattern's type
>>  (as per current specification proposal)
>> -changing the AST structure so that the binding variable has a VariableTree 
>> in the AST. BindingPatternTree is preserved
>>  and encloses the VariableTree. The reason is better consistency in the API, 
>> with nodes like CatchTree, EnhancedForLoop
>>  Tree, etc.
>> 
>> This change will not be integrated until JEP 394 is targetted.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev
> excludes the unrelated changes brought in by the merge/rebase. The pull 
> request contains 15 additional commits since
> the last revision:
>  - Cleanup: using a null instead of List.of() as a parameter to 
> JavaCompiler.getTask
>  - Merge branch 'master' into patterns-instanceof3
>  - Fixing more tests.
>  - Correcting positions.
>  - Improve the AST model.
>  - Merge branch 'master' into patterns-instanceof3
>  - Updating @since tags.
>  - Merge branch 'master' into patterns-instanceof3
>  - Cleaning up preview comments in javadoc.
>  - Merge branch 'master' into patterns-instanceof3
>  - ... and 5 more: 
> https://git.openjdk.java.net/jdk/compare/062b3b15...5978bca0

Changes requested by vromero (Reviewer).

src/jdk.compiler/share/classes/com/sun/source/tree/PatternTree.java line 34:

> 32:  * @since 16
> 33:  */
> 34: public interface PatternTree extends Tree {}

I think that this interface is there for forward compatibility, it is still 
weird to have an empty interface and then
an empty class in JCTree implementing this interface. Doesn't it make sense to 
move any method from BindingPatternTree
up to this interface? I think that if having this empty interface is deemed 
unavoidable then, just an idea, we should
acknowledge the fact that this is a forward looking interface.

src/jdk.compiler/share/classes/com/sun/source/tree/PatternTree.java line 29:

> 27:
> 28: /**
> 29:  * A tree node used as the base class for the different kinds of

the javadoc seems to need an update

-

PR: https://git.openjdk.java.net/jdk/pull/559

Re: RFR: 8250625: Compiler implementation of Pattern Matching for instanceof (Final) [v2]

[Vicente Romero]

On Tue, 20 Oct 2020 11:36:41 GMT, Jan Lahoda  wrote:

>> test/langtools/tools/javac/patterns/LocalVariableTable.java line 29:
>> 
>>> 27:  * @summary Ensure the LV table entries are generated for bindings
>>> 28:  * @modules jdk.jdeps/com.sun.tools.classfile
>>> 29:  * @compile -g LocalVariableTable.java
>> 
>> I believe all tests are always compiled with `-g` option set, not related to 
>> your patch but we could probably remove
>> that line as superfluous
>
> I believe plain jtreg invocations may not be always setting "-g". So probably 
> better to be explicitly clear we need -g,
> as the test itself requires the debugging info/LocalVariableTable?

sure I understand that the `-g` option is necessary. I was just wondering if 
`-g` is not passed always by jtreg. If the
option is passed by default then it is superfluous, although just a comment, I 
understand that it is safer to pass it

-

PR: https://git.openjdk.java.net/jdk/pull/559

Re: RFR: 8250625: Compiler implementation of Pattern Matching for instanceof (Final) [v4]

[Vicente Romero]

On Thu, 22 Oct 2020 12:14:42 GMT, Jan Lahoda  wrote:

>> This is the current proposed patch for the upcoming JEP 394, for pattern 
>> matching for instanceof.
>> 
>> A summary of changes:
>> -making the feature permanent (non-preview)
>> -making the binding variables non-final (as per current specification 
>> proposal)
>> -producing a compile-time error for the case where the expression's type is 
>> a subtype of the type test pattern's type (as per current specification 
>> proposal)
>> -changing the AST structure so that the binding variable has a VariableTree 
>> in the AST. BindingPatternTree is preserved and encloses the VariableTree. 
>> The reason is better consistency in the API, with nodes like CatchTree, 
>> EnhancedForLoop Tree, etc.
>> 
>> This change will not be integrated until JEP 394 is targetted.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge 
> or a rebase. The incremental webrev excludes the unrelated changes brought in 
> by the merge/rebase. The pull request contains 18 additional commits since 
> the last revision:
> 
>  - Removing the preview deprecated methods from BindingPatternTree.
>  - Merge branch 'master' into patterns-instanceof3
>  - Fixing review comments.
>  - Cleanup: using a null instead of List.of() as a parameter to 
> JavaCompiler.getTask
>  - Merge branch 'master' into patterns-instanceof3
>  - Fixing more tests.
>  - Correcting positions.
>  - Improve the AST model.
>  - Merge branch 'master' into patterns-instanceof3
>  - Updating @since tags.
>  - ... and 8 more: 
> https://git.openjdk.java.net/jdk/compare/68eec7af...77468e24

looks good!

-

Marked as reviewed by vromero (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/559

Re: [PATCH] Some improvements for java.lang.Class

[Vicente Romero]

Hi,

From the benchmark data, it seems that the patched code is not a lot 
much faster than the original code, plus as Peter mentioned java.base is 
not compiled with the -XDstringConcat=inline option, so there is no way 
the compiler will generate any indy for the patched code. The new code 
is more compact though, that's its main benefit


Vicente

On 3/26/19 4:15 PM, Сергей Цыпанов wrote:

Hello Peter,

I was unaware of mentioned detail (thank you a lot for pointing that out, it 
made me read the whole JEP280 again) so I've rebenchmarked my changes compiled 
with -XDstringConcat=inline.

1) as of Class::getTypeName for Object[] it turned out, that String::repeat 
performs slightly better:

Benchmark Mode  Cnt 
Score Error   Units

getTypeName_patched   avgt   25
36,676 ±   3,559   ns/op
getTypeName   avgt   25
39,083 ±   1,737   ns/op

getTypeName_patched:·gc.alloc.rate.norm   avgt   25   
152,000 ±   0,001B/op
getTypeName:·gc.alloc.rate.norm   avgt   25   
152,000 ±   0,001B/op


But for Object[][] situation is the opposite:

 Mode  Cnt 
Score Error   Units

getTypeName_patched avgt   5047,045 
±   0,455   ns/op
getTypeName avgt   5040,536 
±   0,196   ns/op

getTypeName_patched:·gc.alloc.rate.norm avgt   50   176,000 
±   0,001B/op
getTypeName:·gc.alloc.rate.norm avgt   50   152,000 
±   0,001B/op


2) as of Class::methodToString patched version clearly wins:

methodToString_1arg avgt   50   238,476 
±   1,641   ns/op
methodToString_1arg_patched avgt   50   197,900 
±   5,824   ns/op

methodToString_1arg:·gc.alloc.rate.norm avgt   50  1209,600 
±   6,401B/op
methodToString_1arg_patched:·gc.alloc.rate.norm avgt   50   936,000 
±   0,001B/op

methodToString_noArgs   avgt   50   224,054 
±   1,840   ns/op
methodToString_noArgs_patched   avgt   5078,195 
±   0,418   ns/op

methodToString_noArgs:·gc.alloc.rate.norm   avgt   50  1131,200 
±   7,839B/op
methodToString_noArgs_patched:·gc.alloc.rate.norm   avgt   50   408,000 
±   0,001B/op


As Brian suggested I've separated the changes. The patch attached contains only 
the changes for Class::methodToString. They are obviously helpful as they rid 
concatenation as argument of StringBuilder::append call (obvious antipattern to 
me) and skip Stream creation for empty array.

String::repeat obviously requires more investigation, it might turn out we 
don't need it in java.base in majority of use cases or in all of them.

Regards,
Sergei Tsypanov



21.03.2019, 00:56, "Peter Levart" :

Hi Sergei,

I don't know if you are aware that the new invokedynamic based
translation of string concatenation expressions introduced in JDK 9
(http://openjdk.java.net/jeps/280) only applies to code outside
java.base module. java.base module is still compiled with old-style
StringBuilder based translation of string concatenation expressions
because of bootstraping issues.

If your benchmarks bellow are compiled with option
-XDstringConcat=inline to disable JEP280 translation (as java.base
module is), then it's OK. Else you are not benchmarking the right
translation of the code as you intend to change the code in java.base
module.

Regards, Peter

On 3/20/19 7:35 PM, Сергей Цыпанов wrote:

  I had a brief conversation with Brian Goetz which has left off the mailing 
list for some reason. Here's the text:
  ---
  Brian:

  These enhancements seem reasonable; these are exactly the cases that 
String::repeat was intended for. (This is a “is this a reasonable idea” review, 
not a code review.).
  Have you looked for other places where similar transformations could be done?

  ---
  Me:

  Hello,
  after I had realized that looped StringBuilder::append calls can sometimes be 
replaced with String::repeat I requested corresponding inspection in IDEA issue 
tracker.
  Using the inspection I’ve found 129 similar warnings in jdk13 repo.
  Some of them are very promising, e.g. BigDecimal:: toPlainString where 
currently we have


  int trailingZeros = checkScaleNonZero((-(long)scale));
  StringBuilder buf;
  if(intCompact!=INFLATED) {
   buf = new StringBuilder(20+trailingZeros);
   buf.append(intCompact);
  } else {
   String str = intVal.toString();
   buf = new StringBuilder(str.length()+trailingZeros);
   buf.append(str);
  }
  for (int i = 0; i < trailingZeros; i++) {
   buf.append('

Re: [PATCH] Some improvements for java.lang.Class

[Vicente Romero]

Hi Sergei,

I have created issue [1] to track your enhancement. Please cc me in your 
next mails I will be helping you to do the paperwork. Talking about that 
did you sign the OCA, [2]?


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8222151
[2] https://www.oracle.com/technetwork/community/oca-486395.html

RFR: JDK-8222151: refactoring: enhancements to java.lang.Class::methodToString and java.lang.Class::getTypeName

[Vicente Romero]

Please review the enhancements to java.lang.Class proposed by Sergei 
Tsypanov see discussion at [1]. The bug can be found at [2] and the fix 
at [3]. This enhancement is removing uses of StringBuilder in:

    ::toGenericString
    ::methodToString, and
    ::getTypeName

Thanks,
Vicente

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-March/059110.html

[2] https://bugs.openjdk.java.net/browse/JDK-8222151
[3] http://cr.openjdk.java.net/~vromero/8222151/webrev.00/

Re: RFR: JDK-8222151: refactoring: enhancements to java.lang.Class::methodToString and java.lang.Class::getTypeName

[Vicente Romero]

Hi Joe,

Thanks for the review please, I have modified the webrev [1]

[1] http://cr.openjdk.java.net/~vromero/8222151/webrev.01/

On 4/11/19 1:26 PM, Joseph D. Darcy wrote:

Hi Vicente,

For methodToString, factoring out computing the common

    getName() + '.' + name

prefix might be preferable, but otherwise the patch looks fine.

Thanks,

-Joe

On 4/11/2019 10:22 AM, Vicente Romero wrote:
Please review the enhancements to java.lang.Class proposed by Sergei 
Tsypanov see discussion at [1]. The bug can be found at [2] and the 
fix at [3]. This enhancement is removing uses of StringBuilder in:

    ::toGenericString
    ::methodToString, and
    ::getTypeName

Thanks,
Vicente

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-March/059110.html

[2] https://bugs.openjdk.java.net/browse/JDK-8222151
[3] http://cr.openjdk.java.net/~vromero/8222151/webrev.00/

Re: RFR: JDK-8222151: refactoring: enhancements to java.lang.Class::methodToString and java.lang.Class::getTypeName

[Vicente Romero]

Hi Joe,

Thanks for your suggestion, please see [1]

Vicente

[1] http://cr.openjdk.java.net/~vromero/8222151/webrev.02/

On 4/11/19 4:48 PM, Joe Darcy wrote:


Hi Vicente,

That version is better. If one doesn't mind the ?: operator, something 
like the following is possible:


return getName() + '.' + name +
(argTypes == null || argTypes.length == 0) ?
"()":
  Arrays.stream(argTypes)
  .map(c -> c == null ? "null" : c.getName())
  .collect(Collectors.joining(",", "(", ")"));

HTH,

-Joe

On 4/11/2019 1:30 PM, Vicente Romero wrote:

Hi Joe,

Thanks for the review please, I have modified the webrev [1]

[1] http://cr.openjdk.java.net/~vromero/8222151/webrev.01/

On 4/11/19 1:26 PM, Joseph D. Darcy wrote:

Hi Vicente,

For methodToString, factoring out computing the common

    getName() + '.' + name

prefix might be preferable, but otherwise the patch looks fine.

Thanks,

-Joe

On 4/11/2019 10:22 AM, Vicente Romero wrote:
Please review the enhancements to java.lang.Class proposed by 
Sergei Tsypanov see discussion at [1]. The bug can be found at [2] 
and the fix at [3]. This enhancement is removing uses of 
StringBuilder in:

    ::toGenericString
    ::methodToString, and
    ::getTypeName

Thanks,
Vicente

[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-March/059110.html

[2] https://bugs.openjdk.java.net/browse/JDK-8222151
[3] http://cr.openjdk.java.net/~vromero/8222151/webrev.00/

Re: RFR - JDK-8212975 ClassDesc should have a full name method

[Vicente Romero]

looks good to me,

Vicente

On 4/23/19 10:58 AM, Jim Laskey wrote:

Revision based on Joe review

webrev: http://cr.openjdk.java.net/~jlaskey/8212975/webrev-03


On Apr 22, 2019, at 3:34 PM, Jim Laskey  wrote:

ClassDesc.displayName(boolean detail) allows the user the option to fetch the 
fully qualified class name.

webrev: http://cr.openjdk.java.net/~jlaskey/8212975/webrev-02 

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


Thank you.

Cheers,

- Jim

RFR: JDK-8219483: j.l.c.ClassDesc::nested(String, String...) doesn't throw NPE if any arg is null

[Vicente Romero]

Hi,

Please review fix [1] and CSR [2] for [3]. The API for method 
j.l.c.ClassDesc::nested(String, String...) states that it should throw 
NPE if any of the arguments is null. The implementation is not in sync 
with the API and should be corrected,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8223034
[3] https://bugs.openjdk.java.net/browse/JDK-8219483

Re: RFR: JDK-8219483: j.l.c.ClassDesc::nested(String, String...) doesn't throw NPE if any arg is null

[Vicente Romero]

Hi Joe,

Thanks for the review. I have modified the patch, please see [1] . I 
still need a reviewer for the CSR [2],


Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8223034


On 4/26/19 9:32 PM, Joe Darcy wrote:

Hi Vicente,

For purposes of a better exception message, do you want to explicitly 
check moreNestedNames for null in some way before accessing its 
contents? Also, I'd commend the spec be updated slightly to


    @throws NullPointerException if any argument or its contents is 
{@code null}


assuming the desired behavior is a NPE if an element of 
moreNestedNames is null as opposed to ust moreNestedNames itself.


Thanks,

-Joe

On 4/26/2019 9:33 AM, Vicente Romero wrote:

Hi,

Please review fix [1] and CSR [2] for [3]. The API for method 
j.l.c.ClassDesc::nested(String, String...) states that it should 
throw NPE if any of the arguments is null. The implementation is not 
in sync with the API and should be corrected,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8223034
[3] https://bugs.openjdk.java.net/browse/JDK-8219483

Re: RFR: JDK-8219483: j.l.c.ClassDesc::nested(String, String...) doesn't throw NPE if any arg is null

[Vicente Romero]

Hi Joe,

Thanks for the reviews, I have updated the webrev after you suggestion [1],

Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.02/

On 4/30/19 6:00 PM, Joseph D. Darcy wrote:

Hi Vicente,

CSR reviewed.

I suggesting adding a test case for

    cd.nested("good", null)

Thanks,

-Joe

On 4/29/2019 2:41 PM, Vicente Romero wrote:

Hi Joe,

Thanks for the review. I have modified the patch, please see [1] . I 
still need a reviewer for the CSR [2],


Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8223034


On 4/26/19 9:32 PM, Joe Darcy wrote:

Hi Vicente,

For purposes of a better exception message, do you want to 
explicitly check moreNestedNames for null in some way before 
accessing its contents? Also, I'd commend the spec be updated 
slightly to


    @throws NullPointerException if any argument or its contents is 
{@code null}


assuming the desired behavior is a NPE if an element of 
moreNestedNames is null as opposed to ust moreNestedNames itself.


Thanks,

-Joe

On 4/26/2019 9:33 AM, Vicente Romero wrote:

Hi,

Please review fix [1] and CSR [2] for [3]. The API for method 
j.l.c.ClassDesc::nested(String, String...) states that it should 
throw NPE if any of the arguments is null. The implementation is 
not in sync with the API and should be corrected,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8223034
[3] https://bugs.openjdk.java.net/browse/JDK-8219483

Re: RFR: JDK-8219483: j.l.c.ClassDesc::nested(String, String...) doesn't throw NPE if any arg is null

[Vicente Romero]

thanks,

Vicente

On 5/1/19 6:47 PM, Joseph D. Darcy wrote:

Hi Vicente,

Revised version looks good; thanks,

-Joe

On 5/1/2019 1:08 PM, Vicente Romero wrote:

Hi Joe,

Thanks for the reviews, I have updated the webrev after you 
suggestion [1],


Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.02/

On 4/30/19 6:00 PM, Joseph D. Darcy wrote:

Hi Vicente,

CSR reviewed.

I suggesting adding a test case for

    cd.nested("good", null)

Thanks,

-Joe

On 4/29/2019 2:41 PM, Vicente Romero wrote:

Hi Joe,

Thanks for the review. I have modified the patch, please see [1] . 
I still need a reviewer for the CSR [2],


Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8223034


On 4/26/19 9:32 PM, Joe Darcy wrote:

Hi Vicente,

For purposes of a better exception message, do you want to 
explicitly check moreNestedNames for null in some way before 
accessing its contents? Also, I'd commend the spec be updated 
slightly to


    @throws NullPointerException if any argument or its contents 
is {@code null}


assuming the desired behavior is a NPE if an element of 
moreNestedNames is null as opposed to ust moreNestedNames itself.


Thanks,

-Joe

On 4/26/2019 9:33 AM, Vicente Romero wrote:

Hi,

Please review fix [1] and CSR [2] for [3]. The API for method 
j.l.c.ClassDesc::nested(String, String...) states that it should 
throw NPE if any of the arguments is null. The implementation is 
not in sync with the API and should be corrected,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8219483/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8223034
[3] https://bugs.openjdk.java.net/browse/JDK-8219483

RFR: JDK-8223723: j.l.c.MethodTypeDesc.dropParameterTypes​ throws the undocumented exception: IllegalArgumentException

[Vicente Romero]

Please review fix for [1] at [2]. The implementation of method 
java.lang.constant.MethodTypeDesc::dropParameterTypes was throwing a non 
specified exception. The proposed fix is synchronizing the 
implementation with the specification. Please also review the CSR at 
[3]. Check the problem section in the CSR for more details,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223723
[2] http://cr.openjdk.java.net/~vromero/8223767/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8223918

RFR: JDK-8223725: j.l.c.MethodHandleDesc::of throws undocumented exception IllegalArgumentException

[Vicente Romero]

Please review fix [1] for [2] and the corresponding CSR at [3]. The fix 
is just adding a missing @throws at method j.l.c.MethodHandleDesc::of. 
It is a one liner fix,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8223725/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8223725
[3] https://bugs.openjdk.java.net/browse/JDK-8223920

RFR: JDK-8223726: j.l.c.MethodTypeDesc spec should contain precise assertions for one parameter's methods

[Vicente Romero]

Please review fix for [1] at [2], this is a simple fix that is just 
rewording the doc of a couple of methods at 
java.lang.constant.MethodTypeDesc,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223726
[2] http://cr.openjdk.java.net/~vromero/8223726/webrev.00/

RFR: JDK-8223803: j.l.c.MethodTypeDesc::insertParameterTypes​ doesn't control type of parameters

[Vicente Romero]

Please review fix for [1] at [2] and the related CSR at [3]. Method 
java.lang.constant.MethodTypeDesc::insertParameterTypes​ should control 
the type of parameters being inserted. In particular, no parameter can 
be a class descriptor of primitive type |void|. This fix is updating the 
spec of the mentioned method plus it is adding a test to make sure that 
the implementation is in sync with the specification. As mentioned in 
the CSR and apiNote has been added to method 
j.l.c.ConstantDesc::resolveConstantDesc to clarify that a method type 
descriptor can have more than 255 argument slots so at resolution time, 
as a j.l.i.MethodType, errors can be thrown.


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223803
[2] http://cr.openjdk.java.net/~vromero/8223803/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8223997

Re: RFR: JDK-8223726: j.l.c.MethodTypeDesc spec should contain precise assertions for one parameter's methods

[Vicente Romero]

Hi Joe,

Yes you are correct, thanks, I have updated the spec for the method plus 
added a couple of tests to make sure that the right exception is being 
thrown [1],


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8223726/webrev.01/

On 5/15/19 1:45 PM, Joe Darcy wrote:

Hi Vicente,

Should

 180  * @throws NullPointerException if any argument is {@code null}
 181  */
 182 static DirectMethodHandleDesc ofConstructor(ClassDesc owner,
 183 ClassDesc... 
paramTypes) {


be "any argument or its contents is {@code null}", i.e. if the 
elements of paramTypes are null, is NPE thrown?


Otherwise, the change looks good.

Thanks,

-Joe

On 5/15/2019 8:36 AM, Vicente Romero wrote:
Please review fix for [1] at [2], this is a simple fix that is just 
rewording the doc of a couple of methods at 
java.lang.constant.MethodTypeDesc,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223726
[2] http://cr.openjdk.java.net/~vromero/8223726/webrev.00/

Re: RFR: JDK-8223726: j.l.c.MethodTypeDesc spec should contain precise assertions for one parameter's methods

[Vicente Romero]

done, I have restricted the scope of the CSR to method 
`j.l.c.MethodTypeDesc::ofConstructor` only which is the one for which 
the doc change is more important,


Thanks,
Vicente

On 5/16/19 12:41 PM, Joe Darcy wrote:

Hi Vicente,

As the spec now includes a very small change to cover the array 
contents, please update the CSR and I'll quickly approve it.


Thanks,

-Joe

On 5/16/2019 9:19 AM, Vicente Romero wrote:

Hi Joe,

Yes you are correct, thanks, I have updated the spec for the method 
plus added a couple of tests to make sure that the right exception is 
being thrown [1],


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8223726/webrev.01/

On 5/15/19 1:45 PM, Joe Darcy wrote:

Hi Vicente,

Should

 180  * @throws NullPointerException if any argument is {@code 
null}

 181  */
 182 static DirectMethodHandleDesc ofConstructor(ClassDesc owner,
 183 ClassDesc... paramTypes) {

be "any argument or its contents is {@code null}", i.e. if the 
elements of paramTypes are null, is NPE thrown?


Otherwise, the change looks good.

Thanks,

-Joe

On 5/15/2019 8:36 AM, Vicente Romero wrote:
Please review fix for [1] at [2], this is a simple fix that is just 
rewording the doc of a couple of methods at 
java.lang.constant.MethodTypeDesc,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223726
[2] http://cr.openjdk.java.net/~vromero/8223726/webrev.00/

Re: RFR: JDK-8223803: j.l.c.MethodTypeDesc::insertParameterTypes​ doesn't control type of parameters

[Vicente Romero]

ping

On 5/15/19 5:27 PM, Vicente Romero wrote:
Please review fix for [1] at [2] and the related CSR at [3]. Method 
java.lang.constant.MethodTypeDesc::insertParameterTypes​ should 
control the type of parameters being inserted. In particular, no 
parameter can be a class descriptor of primitive type |void|. This fix 
is updating the spec of the mentioned method plus it is adding a test 
to make sure that the implementation is in sync with the 
specification. As mentioned in the CSR and apiNote has been added to 
method j.l.c.ConstantDesc::resolveConstantDesc to clarify that a 
method type descriptor can have more than 255 argument slots so at 
resolution time, as a j.l.i.MethodType, errors can be thrown.


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223803
[2] http://cr.openjdk.java.net/~vromero/8223803/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8223997

Re: RFR: JDK-8223803: j.l.c.MethodTypeDesc::insertParameterTypes​ doesn't control type of parameters

[Vicente Romero]

Hi Roger,

Thanks for the review, I will do the re-wrapping before pushing,

Thanks,
Vicente

On 5/16/19 5:15 PM, Roger Riggs wrote:

Hi Vicente,

Looks ok.
And the CSR too.

Please re-wrap the lines to 80 chars to make diffs easier to read.
ConstantDesc.java and MethodTypeDesc.java

Thanks, Roger

On 05/15/2019 05:27 PM, Vicente Romero wrote:
Please review fix for [1] at [2] and the related CSR at [3]. Method 
java.lang.constant.MethodTypeDesc::insertParameterTypes​ should 
control the type of parameters being inserted. In particular, no 
parameter can be a class descriptor of primitive type |void|. This 
fix is updating the spec of the mentioned method plus it is adding a 
test to make sure that the implementation is in sync with the 
specification. As mentioned in the CSR and apiNote has been added to 
method j.l.c.ConstantDesc::resolveConstantDesc to clarify that a 
method type descriptor can have more than 255 argument slots so at 
resolution time, as a j.l.i.MethodType, errors can be thrown.


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223803
[2] http://cr.openjdk.java.net/~vromero/8223803/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8223997

Re: RFR: JDK-8223723: j.l.c.MethodTypeDesc.dropParameterTypes​ throws the undocumented exception: IllegalArgumentException

[Vicente Romero]

Hi,

I still need a reviewer for this simple patch and CSR,

TIA,
Vicente

On 5/14/19 4:53 PM, Vicente Romero wrote:
Please review fix for [1] at [2]. The implementation of method 
java.lang.constant.MethodTypeDesc::dropParameterTypes was throwing a 
non specified exception. The proposed fix is synchronizing the 
implementation with the specification. Please also review the CSR at 
[3]. Check the problem section in the CSR for more details,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223723
[2] http://cr.openjdk.java.net/~vromero/8223767/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8223918

Re: RFR: JDK-8223723: j.l.c.MethodTypeDesc.dropParameterTypes​ throws the undocumented exception: IllegalArgumentException

[Vicente Romero]

ping

On 5/16/19 6:52 PM, Vicente Romero wrote:

Hi,

I still need a reviewer for this simple patch and CSR,

TIA,
Vicente

On 5/14/19 4:53 PM, Vicente Romero wrote:
Please review fix for [1] at [2]. The implementation of method 
java.lang.constant.MethodTypeDesc::dropParameterTypes was throwing a 
non specified exception. The proposed fix is synchronizing the 
implementation with the specification. Please also review the CSR at 
[3]. Check the problem section in the CSR for more details,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223723
[2] http://cr.openjdk.java.net/~vromero/8223767/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8223918

Re: RFR: JDK-8223725: j.l.c.MethodHandleDesc::of throws undocumented exception IllegalArgumentException

[Vicente Romero]

ping

On 5/14/19 6:37 PM, Vicente Romero wrote:
Please review fix [1] for [2] and the corresponding CSR at [3]. The 
fix is just adding a missing @throws at method 
j.l.c.MethodHandleDesc::of. It is a one liner fix,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8223725/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8223725
[3] https://bugs.openjdk.java.net/browse/JDK-8223920

RFR: JDK-8223914: specification of j.l.c.MethodTypeDesc::of should document better the exceptions thrown

[Vicente Romero]

Please review simple fix for [1] at [2] plus the CSR at [3]. This fix is 
simply documenting all the missing cases in which method 
java.lang.constant.MethodTypeDesc::of can throw exceptions. A test has 
been added to cover the missing cases.


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223914
[2] http://cr.openjdk.java.net/~vromero/8223914/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8224136

Re: RFR: JDK-8223725: j.l.c.MethodHandleDesc::of throws undocumented exception IllegalArgumentException

[Vicente Romero]

Hi Roger,

Thanks for the review, my apologies, I made a mistake an applied the 
change to the wrong method. I have corrected the patch [1] and the CSR 
[2]. I have updated the language used as you suggested,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8223725/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8223920

On 5/17/19 2:05 PM, Roger Riggs wrote:

Hi Vicente,

What's the difference in the IAE exceptions describing IAE's as
being "incorrect formats' vs 'is invalid' or 'not valid'?

Each requires a spec for valid and invalid strings or is undefined.

Can the new IAE use the same language as is used for the ofDescriptor 
method?

Avoiding any question about differences.

69:  Also, is the word 'lookup' overloaded?  Is 'lookup' is 
significant in this context?


Roger


On 05/14/2019 06:37 PM, Vicente Romero wrote:
Please review fix [1] for [2] and the corresponding CSR at [3]. The 
fix is just adding a missing @throws at method 
j.l.c.MethodHandleDesc::of. It is a one liner fix,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8223725/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8223725
[3] https://bugs.openjdk.java.net/browse/JDK-8223920

Re: RFR: JDK-8223723: j.l.c.MethodTypeDesc.dropParameterTypes​ throws the undocumented exception: IllegalArgumentException

[Vicente Romero]

Hi Roger,

Thanks again for the reviews, I have modified the CSR summary after your 
suggestion [1]. I have also fixed the issue with the webrev link [2],


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223918
[2] http://cr.openjdk.java.net/~vromero/8223723/webrev.01/

On 5/17/19 2:18 PM, Roger Riggs wrote:

Hi Vicente,

Method j.l.c.MethodTypeDesc.dropParameterTypes​ throws an exception in 
a case
I would change the CSR summary to "Method 
j.l.c.MethodTypeDesc.dropParameterTypes​ should specify 
IndexOutOfBoundsException"

to focus on the new behavior.

The link to the webrev looks ok, but has the fix for 8223725 instead.

Roger


On 05/16/2019 06:52 PM, Vicente Romero wrote:

Hi,

I still need a reviewer for this simple patch and CSR,

TIA,
Vicente

On 5/14/19 4:53 PM, Vicente Romero wrote:
Please review fix for [1] at [2]. The implementation of method 
java.lang.constant.MethodTypeDesc::dropParameterTypes was throwing a 
non specified exception. The proposed fix is synchronizing the 
implementation with the specification. Please also review the CSR at 
[3]. Check the problem section in the CSR for more details,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223723
[2] http://cr.openjdk.java.net/~vromero/8223767/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8223918

Re: RFR: JDK-8223723: j.l.c.MethodTypeDesc.dropParameterTypes​ throws the undocumented exception: IllegalArgumentException

[Vicente Romero]

Hi Roger,


On 5/17/19 4:53 PM, Roger Riggs wrote:

Hi Vicente,

Looks fine.

Please add a "." at the end of the summary 2nd sentence.


done



BTW, are you aware of the range checking methods in 
java.util.Objects.checkFromToIndex(from, to, length)?

They make it easy avoid to check all the conditions on subranges.


I will take a look at them, thanks for the suggestion,


Thanks, Roger


Thanks for all the reviews so far!
Vicente




On 05/17/2019 04:14 PM, Vicente Romero wrote:

Hi Roger,

Thanks again for the reviews, I have modified the CSR summary after 
your suggestion [1]. I have also fixed the issue with the webrev link 
[2],


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223918
[2] http://cr.openjdk.java.net/~vromero/8223723/webrev.01/

On 5/17/19 2:18 PM, Roger Riggs wrote:

Hi Vicente,

Method j.l.c.MethodTypeDesc.dropParameterTypes​ throws an exception 
in a case
I would change the CSR summary to "Method 
j.l.c.MethodTypeDesc.dropParameterTypes​ should specify 
IndexOutOfBoundsException"

to focus on the new behavior.

The link to the webrev looks ok, but has the fix for 8223725 instead.

Roger


On 05/16/2019 06:52 PM, Vicente Romero wrote:

Hi,

I still need a reviewer for this simple patch and CSR,

TIA,
Vicente

On 5/14/19 4:53 PM, Vicente Romero wrote:
Please review fix for [1] at [2]. The implementation of method 
java.lang.constant.MethodTypeDesc::dropParameterTypes was throwing 
a non specified exception. The proposed fix is synchronizing the 
implementation with the specification. Please also review the CSR 
at [3]. Check the problem section in the CSR for more details,


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223723
[2] http://cr.openjdk.java.net/~vromero/8223767/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8223918

Re: RFR: JDK-8223725: j.l.c.MethodHandleDesc::of throws undocumented exception IllegalArgumentException

[Vicente Romero]

how does it look now?

Thanks,
Vicente

On 5/17/19 3:58 PM, Vicente Romero wrote:

Hi Roger,

Thanks for the review, my apologies, I made a mistake an applied the 
change to the wrong method. I have corrected the patch [1] and the CSR 
[2]. I have updated the language used as you suggested,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8223725/webrev.01/
[2] https://bugs.openjdk.java.net/browse/JDK-8223920

On 5/17/19 2:05 PM, Roger Riggs wrote:

Hi Vicente,

What's the difference in the IAE exceptions describing IAE's as
being "incorrect formats' vs 'is invalid' or 'not valid'?

Each requires a spec for valid and invalid strings or is undefined.

Can the new IAE use the same language as is used for the ofDescriptor 
method?

Avoiding any question about differences.

69:  Also, is the word 'lookup' overloaded?  Is 'lookup' is 
significant in this context?


Roger


On 05/14/2019 06:37 PM, Vicente Romero wrote:
Please review fix [1] for [2] and the corresponding CSR at [3]. The 
fix is just adding a missing @throws at method 
j.l.c.MethodHandleDesc::of. It is a one liner fix,


Thanks,
Vicente

[1] http://cr.openjdk.java.net/~vromero/8223725/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8223725
[3] https://bugs.openjdk.java.net/browse/JDK-8223920

Re: RFR: JDK-8223914: specification of j.l.c.MethodTypeDesc::of should document better the exceptions thrown

[Vicente Romero]

ping,

Thanks,
Vicente

On 5/17/19 12:55 PM, Vicente Romero wrote:
Please review simple fix for [1] at [2] plus the CSR at [3]. This fix 
is simply documenting all the missing cases in which method 
java.lang.constant.MethodTypeDesc::of can throw exceptions. A test has 
been added to cover the missing cases.


Thanks,
Vicente

[1] https://bugs.openjdk.java.net/browse/JDK-8223914
[2] http://cr.openjdk.java.net/~vromero/8223914/webrev.00/
[3] https://bugs.openjdk.java.net/browse/JDK-8224136