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

[Alexandre Boulgakov]

Thanks for reviewing! See my responses inline.

I'll wait on sending another webrev until I've received the rest of your 
comments.

-Sasha

On 8/2/2011 2:19 AM, Xuelei Fan wrote:
>> Please review these JNDI changes.
>> Bug detail: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7072353
>> webrev: http://cr.openjdk.java.net/~mduigou/7072353/0/webrev/
> Thanks for your effort to make JNDI free of compile-warning. The work is
> hard, I appreciate it.
>
> 1. I noticed the copyright date of a few files are unchanged, please
> update them before you push the changes.
I've done that in my local copy but didn't include it in the webrev so 
as to not pollute it.
>
> 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.
>
> 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>

>
>    class BaseNameClassPairEnumeration implements NamingEnumeration<T>
>
> *** com/sun/jndi/ldap/Connection.java
>   251         } catch (ClassNotFoundException |
>   252                  InstantiationException |
>   253                  InvocationTargetException |
>   254                  IllegalAccessException e) {
>
> I like this new try-catch feature!
>
> 4. com/sun/jndi/ldap/LdapCtx.java
> 1194    return (NamingEnumeration)
> 1195        new LdapBindingEnumeration(this, answer, name, cont);
>
>     LdapBindingEnumeration is of type NamingEnumeration, it may be not
> necessary to convert to NamingEnumeration. Do you mean
> NamingEnumeration<Binding>?
>
>         return (NamingEnumeration<Binding>)
>             new LdapBindingEnumeration(this, answer, name, cont);
LdapBindingEnumeration extends LdapNamingEnumeration, which implements 
NamingEnumeration<NameClassPair>. This means we can cast it to 
NamingEnumeration<Binding> directly, but must go through a raw 
intermediate like NamingEnumeration or Object. I can change it to a 
double cast with (NamingEnumeration<Binding>)(NamingEnumeration), if you 
think that would improve readability, or LdapBindingEnumeration and 
LdapNamingEnumeration could be refactored to implement different 
NamingEnumeration<T> interfaces (like I did with 
NameClassPairEnumeration and BindingEnumeration in 
src/share/classes/com/sun/jndi/dns/DnsContext.java).
>
> 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)?
>
>     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>.
>
>     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