[Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]

[Alexandre Boulgakov]

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 at oracle.com
>> To: alexandre.boulgakov at oracle.com
>> Cc: joe.darcy at oracle.com, core-libs-dev at 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 at 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 at oracle.com
>>> To: alexandre.boulgakov at oracle.com
>>> Cc: joe.darcy at oracle.com, core-libs-dev at 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
>>>>>>