[Fwd: Code review request: 7072353 JNDI libraries do not build with javac -Xlint:all -Werror]

[Xuelei Fan]

 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