On 8/4/2011 2:03 AM, Alexandre Boulgakov wrote:
> 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.
>
My fault, I thought Enumeration is able to work with for-each. OK,
forgot it, or if you don't mind, please append line 453 at the end of
line 452.
>>
>> . 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?
Sure, it's OK.
> 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?
I think it's a good idea.
>> 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.
Thanks.
Thanks,
Xuelei
>>
>>>> 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
