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
