Re: RFR: JEP 359: Records (Preview) (full code)

Hi, Thanks a lot to all reviewers. This is the header of the changeset I'm planning to push. There have been a ton of people working on the development and / or the review process and I don't want to forget anyone. Please let me know if I'm missing someone: 8225054: Compiler implementation

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Vicente, I reviewed jvm.h, jvm.cpp, and the changes in java.base but only skimmed on the serialization change from this version: http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01/ Class::getRecordComponents    - JVM_GetRecordComponents creates a new RecordComponent

Re: RFR: JEP 359: Records (Preview) (full code)

Hi David, On 12/2/19 7:36 PM, David Holmes wrote: Hi Vicente, I have also re-examined all the hotspot related code and everything looks fine - and very neat (kudos to you and Harold!). thanks, yep Harold has made most of the work here, I just wrote the initial prototype a while ago, A

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Joe, > On 3 Dec 2019, at 05:36, Joe Darcy wrote: > > ... > > As a non-essential cleanup, in > src/java.base/share/classes/java/io/ObjectOutputStream.java: > > 1444 final boolean isRecord = isRecord(obj.getClass()) ? true : > false; > 1445 if (isRecord) { > 1446

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Vicente, These comments are based on http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01/ I looked over the core-libs and javax.lang.model changes. As a non-essential cleanup, in src/java.base/share/classes/java/io/ObjectOutputStream.java: 1444 final

Re: RFR: JEP 359: Records (Preview) (full code)

On 3/12/2019 11:08 am, John Rose wrote: On Dec 2, 2019, at 4:36 PM, David Holmes > wrote: You are using CHECK_0 in RecordComponent::create but that method returns oop. That seems wrong to me, but I see many other oop returning methods also use CHECK_0 so I'll

Re: RFR: JEP 359: Records (Preview) (full code)

On Dec 2, 2019, at 4:36 PM, David Holmes wrote: > > You are using CHECK_0 in RecordComponent::create but that method returns oop. > That seems wrong to me, but I see many other oop returning methods also use > CHECK_0 so I'll take that up separately. (It's only a cosmetic issue.) CHECK_NULL

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Vicente, I have also re-examined all the hotspot related code and everything looks fine - and very neat (kudos to you and Harold!). A couple of nits: src/hotspot/share/classfile/classFileParser.cpp src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp Typo: + //u2 attributs_count;

Re: RFR: JEP 359: Records (Preview) (full code)

I've looked over the hotspot code again as well.  Looks good, you have my review! Lois On 12/2/2019 11:52 AM, coleen.phillim...@oracle.com wrote: (resending to all the lists) Hi, I've looked at the hotspot code again, and it looks good. Nice work, Harold and Vincente! Coleen On 11/27/19

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Vicente, I’ve looked at the Javadoc changes again, everything looks good to me. Hannes > Am 28.11.2019 um 05:37 schrieb Vicente Romero : > > Hi, > > Please review the code for the records feature at [1]. This webrev includes > all: APIs, runtime, compiler, serialization, javadoc, and

Re: RFR: JEP 359: Records (Preview) (full code)

thanks Joe, Vicente On 12/2/19 1:17 AM, Joe Darcy wrote: Hi Vicente, I took the liberty of adding the necessary directory-level jtreg config files to enable the feature and updated the tests accordingly:     https://hg.openjdk.java.net/amber/amber/rev/c91826d62310 (The feature is enabled

Re: RFR: JEP 359: Records (Preview) (full code)

(resending to all the lists) Hi, I've looked at the hotspot code again, and it looks good.  Nice work, Harold and Vincente! Coleen On 11/27/19 11:37 PM, Vicente Romero wrote: Hi, Please review the code for the records feature at [1]. This webrev includes all: APIs, runtime, compiler,

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Vicente, I took the liberty of adding the necessary directory-level jtreg config files to enable the feature and updated the tests accordingly:     https://hg.openjdk.java.net/amber/amber/rev/c91826d62310 (The feature is enabled by default in for the langtools tests, but disabled by

Re: RFR: JEP 359: Records (Preview) (full code)

Looks good - but you need diagnostic fragments for the "canonical", "compact" diagnostic bits (no need for re-review). Maurizio On 30/11/2019 18:29, Vicente Romero wrote: Hi, I have created another iteration at [1], this is just a delta from last iteration [2]. Some comments below [1]

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Jan, I have addressed your comments on delta iteration [1] [1] http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01_delta.01/ some comments below. On 11/29/19 11:30 AM, Jan Lahoda wrote: Hi Vcente, Overall, looks fine I think. A few comments on the javac

Re: RFR: JEP 359: Records (Preview) (full code)

Hi, I have created another iteration at [1], this is just a delta from last iteration [2]. Some comments below [1] http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01_delta.01/ [2] http://cr.openjdk.java.net/~vromero/records.review/all_code/webrev.01 On 11/28/19 1:35 PM,

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Joe, All the tests that have an explicit -source 14 are that way because of, I think to remember, a bug in jtreg that doesn't expand the ${some.property} macro for those tests. I don't remember the details though Thanks, Vicente On 11/29/19 9:59 AM, Joe Darcy wrote: Hi Vicente, Please

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Vcente, Overall, looks fine I think. A few comments on the javac implementation: -in TypeEnter, I believe this: memberEnter.memberEnter(tree.defs.diff(List.convert(JCTree.class, defsBeforeAddingNewMembers)), env); is unnecessary (and fairly slow). defsBeforeAddingNewMembers is initialized

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Vicente, Please change all uses of     @compile --enable-preview -source 14 in jtreg tags to to     @compile --enable-preview -source ${jdk.version} The former structure will spuriously fail when the JDK 14 -> 15 transition occurs. Also, publishing delta-webrevs between iterations in

Re: RFR: JEP 359: Records (Preview) (full code)

Hi Vicente, generally looks good - few comments below; I tried to focus on areas where the compiler code seemed to diverge from the spec, as well on pieces of code which look a leftover from previous spec rounds. * canonical constructors *can* have return statements - compact constructors

Re: RFR: JEP 359: Records (Preview) (full code)

Hi again, Sorry but I realized that I forgot to remove some code on the compiler side. The code removed is small, before we were issuing an error if some serialization methods were declared as record members. That section was removed from the spec. I have prepared another iteration with this

RFR: JEP 359: Records (Preview) (full code)

Hi, Please review the code for the records feature at [1]. This webrev includes all: APIs, runtime, compiler, serialization, javadoc, and more! Must of the code has been reviewed but there have been some changes since reviewers saw it. Also this is the first time an integral webrev is sent