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
