Re: JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
On 7/24/2020 10:32 AM, Sean Mullan wrote: On 7/24/20 1:18 PM, Joe Darcy wrote: - src/jdk.security.jgss/share/classes/com/sun/security/jgss/GSSUtil.java 37 /** 38 * Do not call. 39 */ 40 @Deprecated(since="16", forRemoval=true) 41 public GSSUtil() {} Is your concern that there may be some code out there that might be erroneously using the default constructor so you want to give some warning first before making this a private ctor after it is removed? (I think I agree, but I want to make sure I understand your thinking). That is my concern here, yes. We've had a number of other cases in the JDK where the default constructor appeared in an static-only class as an attractive nuisance. Before static imports, subclassing such a class was a way to get its names in the namespace of the subclass, but that is an anti-pattern that should not be encouraged. Ok, sounds good. src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java 87 * Creates a {@code NTLoginModule}. s/a/an/ Hmm. If the this is pronounced as "N T Login Module", "N" would be a consonant sound so should be prefixed by the indefinite article "a", no? This might be a question for one of the grammar experts amongst us. But isn't "N" a vowel sound: "en"? I suppose looked at that way it is :-) I'll make the adjustments, copyrights, etc. and push this later today. Thanks, -Joe
Re: JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
On 7/24/20 1:18 PM, Joe Darcy wrote: - src/jdk.security.jgss/share/classes/com/sun/security/jgss/GSSUtil.java 37 /** 38 * Do not call. 39 */ 40 @Deprecated(since="16", forRemoval=true) 41 public GSSUtil() {} Is your concern that there may be some code out there that might be erroneously using the default constructor so you want to give some warning first before making this a private ctor after it is removed? (I think I agree, but I want to make sure I understand your thinking). That is my concern here, yes. We've had a number of other cases in the JDK where the default constructor appeared in an static-only class as an attractive nuisance. Before static imports, subclassing such a class was a way to get its names in the namespace of the subclass, but that is an anti-pattern that should not be encouraged. Ok, sounds good. src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java 87 * Creates a {@code NTLoginModule}. s/a/an/ Hmm. If the this is pronounced as "N T Login Module", "N" would be a consonant sound so should be prefixed by the indefinite article "a", no? This might be a question for one of the grammar experts amongst us. But isn't "N" a vowel sound: "en"? --Sean src/jdk.security.auth/share/classes/com/sun/security/auth/module/LdapLoginModule.java 359 * Creates a {@code LdapLoginModule}. s/a/an/ Okay. Thanks, -Joe
Re: JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
Hi Sean, On 7/24/2020 4:52 AM, Sean Mullan wrote: Hi Joe, On 7/24/20 1:17 AM, Joe Darcy wrote: Hello, Please review a set of changes to add explicit constructors to replace default (implicit) constructors in various classes in security libs across several modules: JDK-8250246: Address reliance on default constructors in security libs http://cr.openjdk.java.net/~darcy/8250246.0/ CSR: https://bugs.openjdk.java.net/browse/JDK-8250248 (This is a companion to similar changes being made across other libraries in preparation for creating a new javac lint warning.) One of the classes seems to only accidentally have a constructor, so I added that one as terminally deprecated. You have not updated the copyright dates; not sure if that is necessary for this type of change. Right; I run a script that handles the copyright updates before a push -- less clutter in a review like this where the per-file changes is small and reduces the possibility of spurious merge conflicts for the copyright line during the review period. - src/jdk.security.jgss/share/classes/com/sun/security/jgss/GSSUtil.java 37 /** 38 * Do not call. 39 */ 40 @Deprecated(since="16", forRemoval=true) 41 public GSSUtil() {} Is your concern that there may be some code out there that might be erroneously using the default constructor so you want to give some warning first before making this a private ctor after it is removed? (I think I agree, but I want to make sure I understand your thinking). That is my concern here, yes. We've had a number of other cases in the JDK where the default constructor appeared in an static-only class as an attractive nuisance. Before static imports, subclassing such a class was a way to get its names in the namespace of the subclass, but that is an anti-pattern that should not be encouraged. - src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java 87 * Creates a {@code NTLoginModule}. s/a/an/ Hmm. If the this is pronounced as "N T Login Module", "N" would be a consonant sound so should be prefixed by the indefinite article "a", no? - src/jdk.security.auth/share/classes/com/sun/security/auth/module/LdapLoginModule.java 359 * Creates a {@code LdapLoginModule}. s/a/an/ Okay. Thanks, -Joe
Re: JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
Hi Joe, On 7/24/20 1:17 AM, Joe Darcy wrote: Hello, Please review a set of changes to add explicit constructors to replace default (implicit) constructors in various classes in security libs across several modules: JDK-8250246: Address reliance on default constructors in security libs http://cr.openjdk.java.net/~darcy/8250246.0/ CSR: https://bugs.openjdk.java.net/browse/JDK-8250248 (This is a companion to similar changes being made across other libraries in preparation for creating a new javac lint warning.) One of the classes seems to only accidentally have a constructor, so I added that one as terminally deprecated. You have not updated the copyright dates; not sure if that is necessary for this type of change. - src/jdk.security.jgss/share/classes/com/sun/security/jgss/GSSUtil.java 37 /** 38 * Do not call. 39 */ 40 @Deprecated(since="16", forRemoval=true) 41 public GSSUtil() {} Is your concern that there may be some code out there that might be erroneously using the default constructor so you want to give some warning first before making this a private ctor after it is removed? (I think I agree, but I want to make sure I understand your thinking). - src/jdk.security.auth/share/classes/com/sun/security/auth/module/NTLoginModule.java 87 * Creates a {@code NTLoginModule}. s/a/an/ - src/jdk.security.auth/share/classes/com/sun/security/auth/module/LdapLoginModule.java 359 * Creates a {@code LdapLoginModule}. s/a/an/ --Sean
JDK 16 RFR of JDK-8250246: Address reliance on default constructors in security libs
Hello, Please review a set of changes to add explicit constructors to replace default (implicit) constructors in various classes in security libs across several modules: JDK-8250246: Address reliance on default constructors in security libs http://cr.openjdk.java.net/~darcy/8250246.0/ CSR: https://bugs.openjdk.java.net/browse/JDK-8250248 (This is a companion to similar changes being made across other libraries in preparation for creating a new javac lint warning.) One of the classes seems to only accidentally have a constructor, so I added that one as terminally deprecated. Thanks, -Joe