PS Looking in src/share/classes/com/sun/jndi/dns/DnsContext.java, for
this code
1085 public Binding nextElement() {
1086 try {
1087 return next();
1088 } catch (NamingException e) {
1089 throw (new java.util.NoSuchElementException(
1090 "javax.naming.NamingException was thrown: " +
1091 e.getMessage()));
1092 }
1093 }
I recommend considering replacing 1089 - 1091 with something like
java.util.NoSuchElementException nsee = new
java.util.NoSuchElementException();
nsee.initCause(e);
throw nsee;
-Joe
Alexandre Boulgakov wrote:
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