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

[Joe Darcy]

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