Re: [Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]
That makes sense. I'll do that with automatic refactoring in Netbeans. Do you want me to send another webrev afterwards? Thanks, Sasha ----- Original Message ----- From: Xuelei.Fan@oracle.com To: alexandre.boulgakov@oracle.com Cc: joe.darcy@oracle.com, core-libs-dev@openjdk.java.net Sent: Monday, August 8, 2011 7:30:09 PM GMT -08:00 US/Canada Pacific Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror] On Aug 9, 2011, at 10:16 AM, Alexandre Boulgakov <alexandre.boulgakov@oracle.com> wrote:
I can change it back to LdapNamingEnumeration. I just thought it would be more consistent with LdapBindingEnumeration and LdapSearchEnumeration. I did not think of the back-porting issue. Would it be better to change AbstractLdapNamingEnumeration to LdapNamingEnumeration, as far as back-porting, since they share the most code? Or do you mean for back-porting code that uses these enumerations?
I would prefer to change it back to LNE, and keep the new ALNE class, since the most confusing place is the code using the LNE, I think. Xuelei
I don't think you need to review the other changes; they are specific change that were already discussed and agreed on.
Thanks, Sasha
----- Original Message ----- From: xuelei.fan@oracle.com To: alexandre.boulgakov@oracle.com Cc: joe.darcy@oracle.com, core-libs-dev@openjdk.java.net Sent: Monday, August 8, 2011 6:58:41 PM GMT -08:00 US/Canada Pacific Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]
On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:
Here's the second version: http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/ <http://cr.openjdk.java.net/%7Emduigou/7072353/2/webrev/>
* Changed LdapResult.referrals to be a Vector<Vector<String>>; * Refactored o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration<T>; o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames<T>; o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration; changed LdapBindingEnumeration and LdapSearchEnumeration accordingly, as well as a few of their consumers; Is it necessary to rename LdapNamingEnumeration to LdapNameClassPairEnumeration? I think it might not good for sustaining work, as when backport a fix from JDK 8 to previous releases, we will have to recognize and rename the class accordingly.
Otherwise, looks fine to me.
* Made a few additional small changes that were discussed.
I did not review other updates. There are too many files, if you want me review all of the changes again, please let me know.
Thanks, Xuelei
Thanks to everyone who reviewed the last version!
Thanks, Sasha
On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:
On 8/3/2011 10:44 PM, Joe Darcy wrote:
David Holmes wrote:
Joe Darcy said the following on 08/04/11 12:33:
David Holmes wrote: >> Using wildcards makes the subtyping work along the type argument >> axis. > > So what is the right fix here? To declare the underlying Vector as > a Vector<?> and cast it to something concrete when needed? It > seems very wrong to me to be inserting raw type casts all through > this code.
It isn't entirely clear to be from a quick inspection of the code what the actual type usage is. A writable general Vector should be a Vector<Object> and Vector just meant for reading should be a Vector<?> (or the equivalent Vector<? extends Object>).
If the type usage is "a sequence of X's and Y's" where X and Y don't have some useful supertype, I would recommend using a somewhat different set of data structures, like a list of type-safe heterogeneous containers or a list of a new package-level XorY class.
Buried in one of Sasha's emails it says:
"The current code uses it to store Strings and Vector<String>s."
Hence it is declared Vector<Object>.
This is looking to me like code that should not have been generified.
Hmm. If String or Vector<String> are the two kinds of stored values, I would recommend Vector<Vector<String>> where a lone string was wrapped in a vector. (Actually, I would recommend a List<List<String>>, but that may be beyond the scope of this cleanup.
I can do Vector<Vector<String>>.
List<List<String>> is probably beyond the scope of removing compiler warnings; getting rid of Vectors and Hashtables in general could take a whole summer by itself, and would probably be best to do as a whole, since it's not always clear at first glance if other classes depend on a particular object being a Vector.
-Sasha
-Joe
On 8/9/2011 3:55 PM, Alexandre Boulgakov wrote:
That makes sense. I'll do that with automatic refactoring in Netbeans. Do you want me to send another webrev afterwards?
Just to confirm, you will change LdapNameClassPairEnumeration back to LdapNamingEnumeration, and will not change the name AbstractLdapNamingEnumeration, right? If the answer is right, I will not need to review the webrev any more. Generally, I will always generate a webrev with the latest update before push the changes. Because in the future, some others may refer to the webrev for other work, such as researching regression failure, backport, etc. Thanks for your cleanup the JNDI code. Xuelei
Thanks, Sasha
----- Original Message ----- From: Xuelei.Fan@oracle.com To: alexandre.boulgakov@oracle.com Cc: joe.darcy@oracle.com, core-libs-dev@openjdk.java.net Sent: Monday, August 8, 2011 7:30:09 PM GMT -08:00 US/Canada Pacific Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]
On Aug 9, 2011, at 10:16 AM, Alexandre Boulgakov <alexandre.boulgakov@oracle.com> wrote:
I can change it back to LdapNamingEnumeration. I just thought it would be more consistent with LdapBindingEnumeration and LdapSearchEnumeration. I did not think of the back-porting issue. Would it be better to change AbstractLdapNamingEnumeration to LdapNamingEnumeration, as far as back-porting, since they share the most code? Or do you mean for back-porting code that uses these enumerations?
I would prefer to change it back to LNE, and keep the new ALNE class, since the most confusing place is the code using the LNE, I think.
Xuelei
I don't think you need to review the other changes; they are specific change that were already discussed and agreed on.
Thanks, Sasha
----- Original Message ----- From: xuelei.fan@oracle.com To: alexandre.boulgakov@oracle.com Cc: joe.darcy@oracle.com, core-libs-dev@openjdk.java.net Sent: Monday, August 8, 2011 6:58:41 PM GMT -08:00 US/Canada Pacific Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]
On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:
Here's the second version: http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/ <http://cr.openjdk.java.net/%7Emduigou/7072353/2/webrev/>
* Changed LdapResult.referrals to be a Vector<Vector<String>>; * Refactored o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration<T>; o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames<T>; o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration; changed LdapBindingEnumeration and LdapSearchEnumeration accordingly, as well as a few of their consumers; Is it necessary to rename LdapNamingEnumeration to LdapNameClassPairEnumeration? I think it might not good for sustaining work, as when backport a fix from JDK 8 to previous releases, we will have to recognize and rename the class accordingly.
Otherwise, looks fine to me.
* Made a few additional small changes that were discussed.
I did not review other updates. There are too many files, if you want me review all of the changes again, please let me know.
Thanks, Xuelei
Thanks to everyone who reviewed the last version!
Thanks, Sasha
On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:
On 8/3/2011 10:44 PM, Joe Darcy wrote:
David Holmes wrote:
Joe Darcy said the following on 08/04/11 12:33: > David Holmes wrote: >>> Using wildcards makes the subtyping work along the type argument >>> axis. >> >> So what is the right fix here? To declare the underlying Vector as >> a Vector<?> and cast it to something concrete when needed? It >> seems very wrong to me to be inserting raw type casts all through >> this code. > > It isn't entirely clear to be from a quick inspection of the code > what the actual type usage is. A writable general Vector should be > a Vector<Object> and Vector just meant for reading should be a > Vector<?> (or the equivalent Vector<? extends Object>). > > If the type usage is "a sequence of X's and Y's" where X and Y > don't have some useful supertype, I would recommend using a > somewhat different set of data structures, like a list of type-safe > heterogeneous containers or a list of a new package-level XorY class.
Buried in one of Sasha's emails it says:
"The current code uses it to store Strings and Vector<String>s."
Hence it is declared Vector<Object>.
This is looking to me like code that should not have been generified.
Hmm. If String or Vector<String> are the two kinds of stored values, I would recommend Vector<Vector<String>> where a lone string was wrapped in a vector. (Actually, I would recommend a List<List<String>>, but that may be beyond the scope of this cleanup.
I can do Vector<Vector<String>>.
List<List<String>> is probably beyond the scope of removing compiler warnings; getting rid of Vectors and Hashtables in general could take a whole summer by itself, and would probably be best to do as a whole, since it's not always clear at first glance if other classes depend on a particular object being a Vector.
-Sasha
-Joe
On 8/9/2011 2:53 AM, Xuelei Fan wrote:
On 8/9/2011 3:55 PM, Alexandre Boulgakov wrote:
That makes sense. I'll do that with automatic refactoring in Netbeans. Do you want me to send another webrev afterwards?
Just to confirm, you will change LdapNameClassPairEnumeration back to LdapNamingEnumeration, and will not change the name AbstractLdapNamingEnumeration, right? If the answer is right, I will not need to review the webrev any more. Yes, that's what I did.
Generally, I will always generate a webrev with the latest update before push the changes. Because in the future, some others may refer to the webrev for other work, such as researching regression failure, backport, etc. That makes sense. For completeness of the record here's the webrev, http://cr.openjdk.java.net/~mduigou/7072353/3/webrev/ <http://cr.openjdk.java.net/%7Emduigou/7072353/3/webrev/>.
Thanks for your cleanup the JNDI code. Thanks for reviewing!
-Sasha
Xuelei
Thanks, Sasha
----- Original Message ----- From: Xuelei.Fan@oracle.com To: alexandre.boulgakov@oracle.com Cc: joe.darcy@oracle.com, core-libs-dev@openjdk.java.net Sent: Monday, August 8, 2011 7:30:09 PM GMT -08:00 US/Canada Pacific Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]
On Aug 9, 2011, at 10:16 AM, Alexandre Boulgakov<alexandre.boulgakov@oracle.com> wrote:
I can change it back to LdapNamingEnumeration. I just thought it would be more consistent with LdapBindingEnumeration and LdapSearchEnumeration. I did not think of the back-porting issue. Would it be better to change AbstractLdapNamingEnumeration to LdapNamingEnumeration, as far as back-porting, since they share the most code? Or do you mean for back-porting code that uses these enumerations?
I would prefer to change it back to LNE, and keep the new ALNE class, since the most confusing place is the code using the LNE, I think.
Xuelei
I don't think you need to review the other changes; they are specific change that were already discussed and agreed on.
Thanks, Sasha
----- Original Message ----- From: xuelei.fan@oracle.com To: alexandre.boulgakov@oracle.com Cc: joe.darcy@oracle.com, core-libs-dev@openjdk.java.net Sent: Monday, August 8, 2011 6:58:41 PM GMT -08:00 US/Canada Pacific Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]
On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:
Here's the second version: http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/ <http://cr.openjdk.java.net/%7Emduigou/7072353/2/webrev/>
* Changed LdapResult.referrals to be a Vector<Vector<String>>; * Refactored o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration<T>; o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames<T>; o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration; changed LdapBindingEnumeration and LdapSearchEnumeration accordingly, as well as a few of their consumers; Is it necessary to rename LdapNamingEnumeration to LdapNameClassPairEnumeration? I think it might not good for sustaining work, as when backport a fix from JDK 8 to previous releases, we will have to recognize and rename the class accordingly.
Otherwise, looks fine to me.
* Made a few additional small changes that were discussed.
I did not review other updates. There are too many files, if you want me review all of the changes again, please let me know.
Thanks, Xuelei
Thanks to everyone who reviewed the last version!
Thanks, Sasha
On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:
On 8/3/2011 10:44 PM, Joe Darcy wrote:
David Holmes wrote: > Joe Darcy said the following on 08/04/11 12:33: >> David Holmes wrote: >>>> Using wildcards makes the subtyping work along the type argument >>>> axis. >>> So what is the right fix here? To declare the underlying Vector as >>> a Vector<?> and cast it to something concrete when needed? It >>> seems very wrong to me to be inserting raw type casts all through >>> this code. >> It isn't entirely clear to be from a quick inspection of the code >> what the actual type usage is. A writable general Vector should be >> a Vector<Object> and Vector just meant for reading should be a >> Vector<?> (or the equivalent Vector<? extends Object>). >> >> If the type usage is "a sequence of X's and Y's" where X and Y >> don't have some useful supertype, I would recommend using a >> somewhat different set of data structures, like a list of type-safe >> heterogeneous containers or a list of a new package-level XorY class. > Buried in one of Sasha's emails it says: > > "The current code uses it to store Strings and Vector<String>s." > > Hence it is declared Vector<Object>. > > This is looking to me like code that should not have been generified. > Hmm. If String or Vector<String> are the two kinds of stored values, I would recommend Vector<Vector<String>> where a lone string was wrapped in a vector. (Actually, I would recommend a List<List<String>>, but that may be beyond the scope of this cleanup. I can do Vector<Vector<String>>.
List<List<String>> is probably beyond the scope of removing compiler warnings; getting rid of Vectors and Hashtables in general could take a whole summer by itself, and would probably be best to do as a whole, since it's not always clear at first glance if other classes depend on a particular object being a Vector.
-Sasha
-Joe
participants (2)
-
Alexandre Boulgakov
-
Xuelei Fan