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?
. javax/naming/directory/BasicAttribute.java
It's an old coding style issue, would you mind indent the lines in
inner-class ValuesEnumImpl?
On 8/3/2011 2:44 AM, Alexandre Boulgakov wrote:
> 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.
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.
>>
>> 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.
>>
>> 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).
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?
>>
>> 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
