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

[Xuelei Fan]

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 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
>>>>>
>>