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

[Alexandre Boulgakov]

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.

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