[Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]
Alexandre Boulgakov
alexandre.boulgakov at oracle.com
Tue Aug 2 18:47:08 UTC 2011
Thanks for reviewing! Please 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 that 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 cannot 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
More information about the security-dev
mailing list