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 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....@oracle.com To: alexandre.boulga...@oracle.com Cc: joe.da...@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 >>>
