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

[Xuelei Fan]

On 8/4/2011 2:03 AM, Alexandre Boulgakov wrote:
> Please see my responses inline.
> 
> Thanks!
> -Sasha
> 
> On 8/2/2011 9:13 PM, Xuelei Fan wrote:
>> . com/sun/jndi/toolkit/dir/SearchFilter.java
>>   451         for (NamingEnumeration<?>  ve = attr.getAll();
>>   452              ve.hasMore();
>>   453                ) {
>>
>> The update is OK. But the coding style looks uncomfortable. Would you
>> mind change it to use for-each style?
> For-each only works with Iterables. There doesn't seem to be a way to
> get an Iterable out of an Attribute instance, so the only way to use a
> for-each here would be to wrap the Enumeration in an ArrayList using
> Collections.list(). While this might look neater, the contents of the
> Enumeration would have to be copied over, using time and increasing the
> memory footprint. Changing Attribute to implement Iterable would require
> a spec change, and would be beyond the scope of this fix.
> 
My fault, I thought Enumeration is able to work with for-each. OK,
forgot it, or if you don't mind, please append line 453 at the end of
line 452.

>>
>> . javax/naming/directory/BasicAttribute.java
>> It's an old coding style issue, would you mind indent the lines in
>> inner-class ValuesEnumImpl?
> Got it.
>> We (security team) used to update all copyright date range manually. But
>> Alan told me that the release engineers will run a script over the
>> forest periodically to fix up the date range. I like this way, I think
>> we don't have to update the date range in every fix manually any more.
>> It depends on you.
> I'll keep that in mind, but I've already updated the copyright dates in
> my local copy. Would it be fine to include them in the next version of
> the webrev? 
Sure, it's OK.

> Since these are just warning fixes, they probably will not
> be backported.
>>
>>>> 2. src/share/classes/com/sun/jndi/cosnaming/CNCtx.java
>>>>      In javax.naming.Context,  Hashtable<?,?>   is used for context
>>>> environment. In this changes, you use Hashtable<String,
>>>> java.lang.Object>.
>>>>      What do you think if we keep the type of K and V the same as
>>>> Hashtable<?,?>?
>>>>
>>>>      I also noticed the similar inconsistency at:
>>>>      . com/sun/jndi/dns/DnsContext.java:
>>>>          50   Hashtable<Object,Object>   environment;
>>>>      . com/sun/jndi/ldap/LdapCtx.java
>>>>          226  Hashtable<String, java.lang.Object>   envprops = null;
>>> The environment passed to the constructor is still Hashtable<?,?>, so
>>> there shouldn't be any source incompatibility because of this. However,
>>> the environment is accessed within the class under the assumption that
>>> (String, Object) pairs or (Object, Object) pairs can be added. This
>>> means that the environment must be of type Hashtable<String, Object>  or
>>> Hashtable<Object, Object>  at the time Hashtable.put is called. We can
>>> either cast it at every call site or once in the constructor. If I
>>> understand type erasure correctly, these casts will not affect the
>>> bytecode produced since the compiler is smart enough to notice we are
>>> casting a Hashtable to a Hashtable, so the only difference is in terms
>>> of readability and the number of @SuppressWarnings("unchecked")
>>> annotations thta need to be included.
>> Sounds reasonable.
>>
>>>> 3. src/share/classes/com/sun/jndi/dns/DnsContext.java
>>>>      What do you think if we have BaseNameClassPairEnumeration to
>>>> implement the NamingEnumeration, so that we can share the code of
>>>> nextElement()?
>>> I'll change it to
>>> abstract class BaseNameClassPairEnumeration<T>  implements
>>> NamingEnumeration<T>
>>>
>> You may also want to change BaseFlatNames in
>> com/sun/jndi/toolkit/dir/HierMemDirCtx.java with the similar style.
> Got it. Would it make sense to make everything except for the next()
> methods final, or will the compiler infer that?
I think it's a good idea.

>> LdapBindingEnumeration --extends-->  LdapNamingEnumeration --implements
>> -->  NamingEnumeration<NameClassPair>.
>>
>>  From the above hierarchy, LdapBindingEnumeration is an instance of
>> NamingEnumeration<NameClassPair>. Now you cast it to
>> NamingEnumeration<Binding>. It might work, but it looks unreasonable and
>> risky in the first glance.
>>
>> The LdapNamingEnumeration and LdapBindingEnumeration is designed when
>> using non-generic programming style. If we switch to use generic
>> features, the inherit structure may not sound reasonable any more.
>>
>> You made a good update in DnsContext and HierMemDirCtx by introducing a
>> intermediate class, BaseNameClassPairEnumeration/BaseFlatNames. Does it
>> sound a solution to LdapBindingEnumeration and LdapNamingEnumeration?
> Yes, I can do that. And also for LdapSearchEnumeration which extends
> LdapNamingEnumeration as well.
Thanks.

Thanks,
Xuelei
>>
>>>> 2244  switch (propName) {
>>>>      Do you want to indent this block? We usually indent a block
>>>> even for
>>>> switch blocks.
>>> Oops, didn't notice that.
>>>> 3017 Vector<Object>   temp = (Vector)extractURLs(res.errorMessage);
>>>>      You may not need the conversion any more, the return value of
>>>> extractURLs() has been updated to
>>>>      2564     private static Vector<String>   extractURLs(String
>>>> refString)
>>> The cast is needed to go from Vector<String>  to Vector<Object>.
>>>> 5. com/sun/jndi/ldap/LdapBindingEnumeration.java
>>>>      Why it is necessary to convert the return type twice (line 92)?
>>>>
>> 8>>      92   return (LdapBindingEnumeration)(NamingEnumeration)
>>>>      93           refCtx.listBindings(listArg);
>>> Again, it's due to a generic type mismatch: refCtx.listBindings(listArg)
>>> returns a NamingEnumeration<Binding>  but LdapBindingEnumeration
>>> implements NamingEnumeration<NameClassPair>. It'd be great if we could
>>> have variant generic interface type parameters, so
>>> NamingEnumeration<Binding>  could extend
>>> NamingEnumeration<NameClassPair>.
>> See above.
>>
>> Otherwise, looks fine to me.
>>
>> Thanks,
>> Xuelei
>>
>>>>      It's great to use convariant return type. I would suggest add
>>>> override tag to make it easy understood.
>>>>
>>>> I am only able to review a quarter of the update today, will continue
>>>> tomorrow.
>>>>
>>>> Thanks,
>>>> Xuelei