Re: RFR (S): 8241947: Minor comment fixes for system property handling
+1 Mandy On 3/31/20 11:57 AM, Langer, Christoph wrote: Hi Mandy, this is a good suggestion. The listing of system properties at the props field declaration seems somewhat redundant, given that it already exists more exactly and with API normativity in the doc of System::getProperties(). So what do you think of this version: http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ <http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/> ? Thanks Christoph *From:* Mandy Chung *Sent:* Dienstag, 31. März 2020 19:34 *To:* Langer, Christoph ; core-libs-...@openjdk.java.net *Cc:* build-dev *Subject:* Re: RFR (S): 8241947: Minor comment fixes for system property handling On 3/31/20 7:56 AM, Langer, Christoph wrote: Hi, please review a small fix that updates two comments. The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property "vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 (Minimize JNI upcalls in system-properties initialization). The second one is the code comment attached to "private static Properties props;" in java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be undefined), "java.vendor.version" can be undefined, so this should be reflected in the comment. I also took the liberty to remove an unneeded import statement. Bug:https://bugs.openjdk.java.net/browse/JDK-8241947 Webrev:http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/ I wonder if we still want to keep this block of comments listing a subset of system properties. Anyway the minor comment looks okay. Mandy
Re: RFR (S): 8241947: Minor comment fixes for system property handling
My mistake, I mistook it for the spec table. On 4/1/20 12:06 PM, Alan Bateman wrote: On 01/04/2020 17:03, Roger Riggs wrote: Hi, Does dropping the "The following properties are guaranteed to be defined:" would seem to be a spec change. It's a comment on a private field, there's no change to the getProperties spec. -Alan
Re: RFR (S): 8241947: Minor comment fixes for system property handling
On 01/04/2020 17:03, Roger Riggs wrote: Hi, Does dropping the "The following properties are guaranteed to be defined:" would seem to be a spec change. It's a comment on a private field, there's no change to the getProperties spec. -Alan
Re: RFR (S): 8241947: Minor comment fixes for system property handling
Hi, Does dropping the "The following properties are guaranteed to be defined:" would seem to be a spec change. Just linking to the properties page makes no such guarantee. Roger On 4/1/20 10:45 AM, Alan Bateman wrote: On 31/03/2020 19:57, Langer, Christoph wrote: Hi Mandy, this is a good suggestion. The listing of system properties at the props field declaration seems somewhat redundant, given that it already exists more exactly and with API normativity in the doc of System::getProperties(). So what do you think of this version: http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ ? Mandy's suggestion to link to getProperties instead is good. This version looks good to me. -Alan.
RE: RFR (S): 8241947: Minor comment fixes for system property handling
Thanks for the review, Alan. I'll push it then. Best regards Christoph > -Original Message- > From: Alan Bateman > Sent: Mittwoch, 1. April 2020 16:46 > To: Langer, Christoph ; Mandy Chung > ; core-libs-...@openjdk.java.net > Cc: build-dev > Subject: Re: RFR (S): 8241947: Minor comment fixes for system property > handling > > On 31/03/2020 19:57, Langer, Christoph wrote: > > Hi Mandy, > > > > this is a good suggestion. The listing of system properties at the props > > field > declaration seems somewhat redundant, given that it already exists more > exactly and with API normativity in the doc of System::getProperties(). > > > > So what do you think of this version: > http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ ? > > > Mandy's suggestion to link to getProperties instead is good. This > version looks good to me. > > -Alan.
Re: RFR (S): 8241947: Minor comment fixes for system property handling
On 31/03/2020 19:57, Langer, Christoph wrote: Hi Mandy, this is a good suggestion. The listing of system properties at the props field declaration seems somewhat redundant, given that it already exists more exactly and with API normativity in the doc of System::getProperties(). So what do you think of this version: http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ ? Mandy's suggestion to link to getProperties instead is good. This version looks good to me. -Alan.
RE: RFR (S): 8241947: Minor comment fixes for system property handling
Hi Mandy, this is a good suggestion. The listing of system properties at the props field declaration seems somewhat redundant, given that it already exists more exactly and with API normativity in the doc of System::getProperties(). So what do you think of this version: http://cr.openjdk.java.net/~clanger/webrevs/8241947.1/ ? Thanks Christoph From: Mandy Chung Sent: Dienstag, 31. März 2020 19:34 To: Langer, Christoph ; core-libs-...@openjdk.java.net Cc: build-dev Subject: Re: RFR (S): 8241947: Minor comment fixes for system property handling On 3/31/20 7:56 AM, Langer, Christoph wrote: Hi, please review a small fix that updates two comments. The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property "vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 (Minimize JNI upcalls in system-properties initialization). The second one is the code comment attached to "private static Properties props;" in java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be undefined), "java.vendor.version" can be undefined, so this should be reflected in the comment. I also took the liberty to remove an unneeded import statement. Bug: https://bugs.openjdk.java.net/browse/JDK-8241947 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/ I wonder if we still want to keep this block of comments listing a subset of system properties. Anyway the minor comment looks okay. Mandy
RE: RFR (S): 8241947: Minor comment fixes for system property handling
Hi Magnus, > From a build perspective this looks fine. Thanks for the review. > But it seems you are changing the interface for java.lang.System. Don't > you need a CSR for that? Or is your claim that the interface was indeed > changed by JDK-8197927, and it is a bug that the documentation was not > updated to match this? Yes, the second is true. The interface/spec was already changed with JDK-8197927 which also has a CSR attached. The commit is this: http://hg.openjdk.java.net/jdk/jdk/rev/d80becbcd3c1. What I'm modifying here is only the non API relevant doc of the private field props. The actual interface is documented in the Javadoc of method java.lang.System::getProperties(). Cheers Christoph
Re: RFR (S): 8241947: Minor comment fixes for system property handling
On 3/31/20 7:56 AM, Langer, Christoph wrote: Hi, please review a small fix that updates two comments. The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property "vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 (Minimize JNI upcalls in system-properties initialization). The second one is the code comment attached to "private static Properties props;" in java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be undefined), "java.vendor.version" can be undefined, so this should be reflected in the comment. I also took the liberty to remove an unneeded import statement. Bug: https://bugs.openjdk.java.net/browse/JDK-8241947 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/ I wonder if we still want to keep this block of comments listing a subset of system properties. Anyway the minor comment looks okay. Mandy
Re: RFR (S): 8241947: Minor comment fixes for system property handling
From a build perspective this looks fine. But it seems you are changing the interface for java.lang.System. Don't you need a CSR for that? Or is your claim that the interface was indeed changed by JDK-8197927, and it is a bug that the documentation was not updated to match this? /Magnus On 2020-03-31 16:56, Langer, Christoph wrote: Hi, please review a small fix that updates two comments. The first one, in make/autoconf/spec.gmk.in, is probably quite old. It talks about handling of a property "vm.vendor" in VersionProps.java.template. However, there is no property "vm.vendor", it must rather be "java.vendor". I stumbled over that when looking at the change of JDK-4947890 (Minimize JNI upcalls in system-properties initialization). The second one is the code comment attached to "private static Properties props;" in java.lang.System. After JDK-8197927 (Allow the system property `java.vendor.version` to be undefined), "java.vendor.version" can be undefined, so this should be reflected in the comment. I also took the liberty to remove an unneeded import statement. Bug: https://bugs.openjdk.java.net/browse/JDK-8241947 Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8241947.0/ Thanks Christoph
