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

[Xuelei Fan]

> 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.

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.
   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 environment;
   . com/sun/jndi/ldap/LdapCtx.java
   226  Hashtable envprops = null;

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()?

  class BaseNameClassPairEnumeration implements NamingEnumeration

*** 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
1194return (NamingEnumeration)
1195new LdapBindingEnumeration(this, answer, name, cont);

   LdapBindingEnumeration is of type NamingEnumeration, it may be not
necessary to convert to NamingEnumeration. Do you mean
NamingEnumeration?

   return (NamingEnumeration)
   new LdapBindingEnumeration(this, answer, name, cont);

2244  switch (propName) {
   Do you want to indent this block? We usually indent a block even for
switch blocks.

3017 Vector 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 extractURLs(String refString)

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);

   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

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

[Alan Bateman]

Xuelei Fan wrote:

:
1. I noticed the copyright date of a few files are unchanged, please
update them before you push the changes.
  
This has come up a few times but I don't think it is strictly required. 
Kelly or one of the release engineers run a script over the forest 
periodically to fix up the date range. The nice thing (in my view 
anyway) about not changing the headers is that it avoids clutter in the 
patch and webrev. It also makes it a bit easier to transport patches to 
other releases.


-Alan

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

[Xuelei Fan]

On Aug 3, 2011, at 12:11 AM, Alan Bateman  wrote:

> Xuelei Fan wrote:
>> :
>> 1. I noticed the copyright date of a few files are unchanged, please
>> update them before you push the changes.
>>  
> This has come up a few times but I don't think it is strictly required. Kelly 
> or one of the release engineers run a script over the forest periodically to 
> fix up the date range.
It's very good. I like the way that update the header in bunch.

Thanks,
Xuelei

> The nice thing (in my view anyway) about not changing the headers is that it 
> avoids clutter in the patch and webrev. It also makes it a bit easier to 
> transport patches to other releases.
> 
> -Alan
> 

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

[Alexandre Boulgakov]

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.
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  environment;
. com/sun/jndi/ldap/LdapCtx.java
226  Hashtable  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 or 
Hashtable 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 implements 
NamingEnumeration




   class BaseNameClassPairEnumeration implements NamingEnumeration

*** 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
1194return (NamingEnumeration)
1195new LdapBindingEnumeration(this, answer, name, cont);

LdapBindingEnumeration is of type NamingEnumeration, it may be not
necessary to convert to NamingEnumeration. Do you mean
NamingEnumeration?

return (NamingEnumeration)
new LdapBindingEnumeration(this, answer, name, cont);
LdapBindingEnumeration extends LdapNamingEnumeration, which implements 
NamingEnumeration. This means we cannot cast it to 
NamingEnumeration directly, but must go through a raw 
intermediate like NamingEnumeration or Object. I can change it to a 
double cast with (NamingEnumeration)(NamingEnumeration), if you 
think that would improve readability, or LdapBindingEnumeration and 
LdapNamingEnumeration could be refactored to implement different 
NamingEnumeration 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  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  extractURLs(String refString)

The cast is needed to go from Vector to Vector.


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 but LdapBindingEnumeration 
implements NamingEnumeration. It'd be great if we could 
have variant generic interface type parameters, so 
NamingEnumeration could extend NamingEnumeration.


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

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

[Alexandre Boulgakov]

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.


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.
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  environment;
. com/sun/jndi/ldap/LdapCtx.java
226  Hashtable  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 or 
Hashtable 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.


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 implements 
NamingEnumeration




   class BaseNameClassPairEnumeration implements NamingEnumeration

*** 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
1194return (NamingEnumeration)
1195new LdapBindingEnumeration(this, answer, name, cont);

LdapBindingEnumeration is of type NamingEnumeration, it may be not
necessary to convert to NamingEnumeration. Do you mean
NamingEnumeration?

return (NamingEnumeration)
new LdapBindingEnumeration(this, answer, name, cont);
LdapBindingEnumeration extends LdapNamingEnumeration, which implements 
NamingEnumeration. This means we can cast it to 
NamingEnumeration directly, but must go through a raw 
intermediate like NamingEnumeration or Object. I can change it to a 
double cast with (NamingEnumeration)(NamingEnumeration), if you 
think that would improve readability, or LdapBindingEnumeration and 
LdapNamingEnumeration could be refactored to implement different 
NamingEnumeration 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  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  extractURLs(String refString)

The cast is needed to go from Vector to Vector.


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 but LdapBindingEnumeration 
implements NamingEnumeration. It'd be great if we could 
have variant generic interface type parameters, so 
NamingEnumeration could extend NamingEnumeration.


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

Re: [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> 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  environment;
>> . com/sun/jndi/ldap/LdapCtx.java
>> 226  Hashtable  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 or
> Hashtable 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 implements
> NamingEnumeration
> 
You may also want to change BaseFlatNames in
com/sun/jndi/toolkit/dir/HierMemDirCtx.java with the similar style.

>>
>>class BaseNameClassPairEnumeration implements NamingEnumeration
>>
>> *** 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
>> 1194return (NamingEnumeration)
>> 1195new LdapBindingEnumeration(this, answer, name, cont);
>>
>> LdapBindingEnumeration is of type NamingEnumeration, it may be not
>> necessary to convert to NamingEnumeration. Do you mean
>> NamingEnumeration?
>>
>> return (NamingEnumeration)
>> new LdapBindingEnumeration(this, answer, name, cont);
> LdapBindingEnumeration extends LdapNamingEnumeration, which implements
> NamingEnumeration. This means we can cast it to
> NamingEnumeration directly, but must go through a raw
> intermediate like NamingEnumeration or Object. I can change it to a
> double cast with (NamingEnumeration)(NamingEnumeration), if you
> think that would improve readability, or LdapBindingEnumeration and
> LdapNamingEnumeration could be refactored to implement different
> NamingEnumeration interfaces (like I did with
> NameClassPairEnumeration and BindingEnumeration in
> src/share/classes/com/sun/jndi/dns/DnsContext.java).

LdapBindingEnumeration --extends--> LdapNamingEnumeration --implements
--> NamingEnumeration.

>From the above hierarchy, LdapBindingEnumeration is an instance of
NamingEnumeration. Now you cast it to
NamingEnumeration. 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

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

[David Holmes]

Alexandre Boulgakov said the following on 08/03/11 04:44:

On 8/2/2011 2:19 AM, Xuelei Fan wrote:

3017 Vector  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  extractURLs(String refString)

The cast is needed to go from Vector to Vector.


Raw types should be avoided (here and elsewhere there are casts to raw 
Vector). I'm surprised (generics continue to surprise me) that despite 
all our advances in type-inference etc that the compiler can not tell 
that a Vector is-a Vector. :(


Not knowing how LdapResult.referrals is used overall I'm unsure what 
might be a better fix but perhaps referrals could be declared as 
Vector and an unchecked cast to Vector added only/if where 
needed?


Cheers,
David Holmes

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

[Joe Darcy]

On 8/3/2011 12:42 AM, David Holmes wrote:

Alexandre Boulgakov said the following on 08/03/11 04:44:

On 8/2/2011 2:19 AM, Xuelei Fan wrote:

3017 Vector  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  extractURLs(String 
refString)

The cast is needed to go from Vector to Vector.


Raw types should be avoided (here and elsewhere there are casts to raw 
Vector). I'm surprised (generics continue to surprise me) that despite 
all our advances in type-inference etc that the compiler can not tell 
that a Vector is-a Vector. :(


That is because in general a Vector is not a Vector because 
of the way subtyping works.  As with arrays, it all looks fine until you 
want to change the container; consider


Vector vs = new Vector<>();
...
Vector vo = vs; // Assume this was okay to alias an object 
vector and a string vector


vo.add(new Integer(1));  // Add an Integer to a list of strings, boom!

Using wildcards makes the subtyping work along the type argument axis.

-Joe

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

[Dr Andrew John Hughes]

On 09:24 Wed 03 Aug , Joe Darcy wrote:
> On 8/3/2011 12:42 AM, David Holmes wrote:
> > Alexandre Boulgakov said the following on 08/03/11 04:44:
> >> On 8/2/2011 2:19 AM, Xuelei Fan wrote:
> >>> 3017 Vector  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  extractURLs(String 
> >>> refString)
> >> The cast is needed to go from Vector to Vector.
> >
> > Raw types should be avoided (here and elsewhere there are casts to raw 
> > Vector). I'm surprised (generics continue to surprise me) that despite 
> > all our advances in type-inference etc that the compiler can not tell 
> > that a Vector is-a Vector. :(
> 
> That is because in general a Vector is not a Vector because 
> of the way subtyping works.  As with arrays, it all looks fine until you 
> want to change the container; consider
> 
> Vector vs = new Vector<>();
> ...
> Vector vo = vs; // Assume this was okay to alias an object 
> vector and a string vector
> 
> vo.add(new Integer(1));  // Add an Integer to a list of strings, boom!
> 
> Using wildcards makes the subtyping work along the type argument axis.
> 

Exactly.  What I wondered on reading this is why it needs to be cast to a
Vector anyway.  That would only reduce type safety as you say.

> -Joe
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37

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

[Alexandre Boulgakov]

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.
 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   environment;
 . com/sun/jndi/ldap/LdapCtx.java
 226  Hashtable   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  or
Hashtable  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  implements
NamingEnumeration


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.

 From the above hierarchy, LdapBindingEnumeration is an instance of
NamingEnumeration. Now you cast it to
NamingEnumeration. 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   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   extractURLs(String refString)

The cast is needed to go from Vector  to Vector.

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  but LdapBindingEnumeration
implements NamingEnumeration. It'd be great if we could
have variant generic interfac

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

[Alexandre Boulgakov]

On 8/3/2011 10:51 AM, Dr Andrew John Hughes wrote:

On 09:24 Wed 03 Aug , Joe Darcy wrote:

On 8/3/2011 12:42 AM, David Holmes wrote:

Alexandre Boulgakov said the following on 08/03/11 04:44:

On 8/2/2011 2:19 AM, Xuelei Fan wrote:

3017 Vector   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   extractURLs(String
refString)

The cast is needed to go from Vector  to Vector.

Raw types should be avoided (here and elsewhere there are casts to raw
Vector). I'm surprised (generics continue to surprise me) that despite
all our advances in type-inference etc that the compiler can not tell
that a Vector  is-a Vector. :(

That is because in general a Vector  is not a Vector  because
of the way subtyping works.  As with arrays, it all looks fine until you
want to change the container; consider

Vector  vs = new Vector<>();
...
Vector  vo = vs; // Assume this was okay to alias an object
vector and a string vector

vo.add(new Integer(1));  // Add an Integer to a list of strings, boom!

Using wildcards makes the subtyping work along the type argument axis.


Exactly.  What I wondered on reading this is why it needs to be cast to a
Vector  anyway.  That would only reduce type safety as you say.
The current code uses it to store Strings and Vectors. The most 
specific common base type is Object, so I don't think we can do any 
better than that.



-Joe

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

[Neil Richards]

On Wed, 2011-08-03 at 11:03 -0700, 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.

Would it  be useful to have a utility object to convert an Enumeration
so it can be used in for-each constructs? - something like:

import java.util.Enumeration;
import java.util.Iterator;

/**
 * Utility class to transform an Enumeration object such that it can be 
used in
 * the for-each construct.
 */
public class IterableEnumerationHolder implements Iterable, 
Iterator {
private final Enumeration e;

public IterableEnumerationHolder(final Enumeration e) {
this.e = e;
}

@Override
public Iterator iterator() {
return this;
}

@Override
public boolean hasNext() {
return e.hasMoreElements();
}

@Override
public E next() {
return e.nextElement();
}

@Override
public void remove() {
throw new UnsupportedOperationException();
}
}


If it would, perhaps it might be considered for Java 8?

Regards,
Neil

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

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

[Alexandre Boulgakov]

Users of Iterable expect to call Iterable.iterator() multiple times, 
receiving a fresh iterator pointing to the beginning of the Iterable 
each time. This would not be possible to do with an Enumeration wrapper 
since Enumerations are read-once.


I don't know if this is a strong enough reason not to include such a 
utility class, but it could certainly be confusing.


-Sasha

On 8/3/2011 2:09 PM, Neil Richards wrote:

On Wed, 2011-08-03 at 11:03 -0700, 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.

Would it  be useful to have a utility object to convert an Enumeration
so it can be used in for-each constructs? - something like:

 import java.util.Enumeration;
 import java.util.Iterator;

 /**
  * Utility class to transform an Enumeration object such that it can 
be used in
  * the for-each construct.
  */
 public class IterableEnumerationHolder  implements Iterable, 
Iterator  {
 private final Enumeration  e;

 public IterableEnumerationHolder(final Enumeration  e) {
 this.e = e;
 }

 @Override
 public Iterator  iterator() {
 return this;
 }

 @Override
 public boolean hasNext() {
 return e.hasMoreElements();
 }

 @Override
 public E next() {
 return e.nextElement();
 }

 @Override
 public void remove() {
 throw new UnsupportedOperationException();
 }
 }


If it would, perhaps it might be considered for Java 8?

Regards,
Neil

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

[Xuelei Fan]

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>>> 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   environment;
  . com/sun/jndi/ldap/LdapCtx.java
  226  Hashtable   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  or
>>> Hashtable  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  implements
>>> NamingEnumeration
>>>
>> 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.
>>
>>  From the above hierarchy, LdapBindingEnumeration is an instance of
>> NamingEnumeration. Now you cast it to
>> NamingEnumeration. 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   temp = (Vector)extractURLs(res.errorMessage);
  You may not need the conversion any more, the return value of
 extractURLs() has b

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

[David Holmes]

Joe Darcy said the following on 08/04/11 02:24:

On 8/3/2011 12:42 AM, David Holmes wrote:

Alexandre Boulgakov said the following on 08/03/11 04:44:

On 8/2/2011 2:19 AM, Xuelei Fan wrote:

3017 Vector  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  extractURLs(String 
refString)

The cast is needed to go from Vector to Vector.


Raw types should be avoided (here and elsewhere there are casts to raw 
Vector). I'm surprised (generics continue to surprise me) that despite 
all our advances in type-inference etc that the compiler can not tell 
that a Vector is-a Vector. :(


That is because in general a Vector is not a Vector because 
of the way subtyping works.  As with arrays, it all looks fine until you 
want to change the container; consider


Vector vs = new Vector<>();
...
Vector vo = vs; // Assume this was okay to alias an object 
vector and a string vector


vo.add(new Integer(1));  // Add an Integer to a list of strings, boom!


I see what you are saying but with arrays the boom would be an 
ArrayStoreException would it not? So the problem here is that there is 
no runtime checking for generics that could do the same. So we have to 
disallow this.



Using wildcards makes the subtyping work along the type argument axis.


So what is the right fix here? To declare the underlying Vector as a 
Vector and cast it to something concrete when needed? It seems very 
wrong to me to be inserting raw type casts all through this code.


Cheers,
David


-Joe

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

[Joe Darcy]

David Holmes wrote:

Joe Darcy said the following on 08/04/11 02:24:

On 8/3/2011 12:42 AM, David Holmes wrote:

Alexandre Boulgakov said the following on 08/03/11 04:44:

On 8/2/2011 2:19 AM, Xuelei Fan wrote:

3017 Vector  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  extractURLs(String 
refString)

The cast is needed to go from Vector to Vector.


Raw types should be avoided (here and elsewhere there are casts to 
raw Vector). I'm surprised (generics continue to surprise me) that 
despite all our advances in type-inference etc that the compiler can 
not tell that a Vector is-a Vector. :(


That is because in general a Vector is not a Vector 
because of the way subtyping works.  As with arrays, it all looks 
fine until you want to change the container; consider


Vector vs = new Vector<>();
...
Vector vo = vs; // Assume this was okay to alias an object 
vector and a string vector


vo.add(new Integer(1));  // Add an Integer to a list of strings, boom!


I see what you are saying but with arrays the boom would be an 
ArrayStoreException would it not? So the problem here is that there is 
no runtime checking for generics that could do the same. So we have to 
disallow this.


Exactly; since arrays are reified so the VM has the information needed 
to check for an errant store and when appropriate throw the 
ArrayStoreException.  Since generics are implemented via erasure, the 
analogous information is not available at runtime for collections.  Note 
that the behavior of arrays is statically unsafe and requires the 
runtime check.





Using wildcards makes the subtyping work along the type argument axis.


So what is the right fix here? To declare the underlying Vector as a 
Vector and cast it to something concrete when needed? It seems very 
wrong to me to be inserting raw type casts all through this code.


It isn't entirely clear to be from a quick inspection of the code what 
the actual type usage is.  A writable general Vector should be a 
Vector and Vector just meant for reading should be a Vector 
(or the equivalent Vector).


If the type usage is "a sequence of X's and Y's" where X and Y don't 
have some useful supertype, I would recommend using a somewhat different 
set of data structures, like a list of type-safe heterogeneous 
containers or a list of a new package-level XorY class.


-Joe

Re: [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 Hashtablejava.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  environment;
. com/sun/jndi/ldap/LdapCtx.java
226  Hashtable  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 or Hashtable 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 implements 
NamingEnumeration




   class BaseNameClassPairEnumeration implements NamingEnumeration

*** 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
1194return (NamingEnumeration)
1195new LdapBindingEnumeration(this, answer, name, cont);

LdapBindingEnumeration is of type NamingEnumeration, it may be not
necessary to convert to NamingEnumeration. Do you mean
NamingEnumeration?

return (NamingEnumeration)
new LdapBindingEnumeration(this, answer, name, cont);
LdapBindingEnumeration extends LdapNamingEnumeration, which implements 
NamingEnumeration. This means we cannot cast it to 
NamingEnumeration directly, but must go through a raw 
intermediate like NamingEnumeration or Object. I can change it to a 
double cast with (NamingEnumeration)(NamingEnumeration), if 
you think that would improve readability, or LdapBindingEnumeration 
and LdapNamingEnumeration could be refactored to implement different 
NamingEnumeration 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  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  extractURLs(String 
refString)

The cast is needed to go from Vector to Vector.


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 but 
LdapBindingEnumeration implements NamingEnumeration. 
It'd be great if we could have var

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

[David Holmes]

Joe Darcy said the following on 08/04/11 12:33:

David Holmes wrote:

Using wildcards makes the subtyping work along the type argument axis.


So what is the right fix here? To declare the underlying Vector as a 
Vector and cast it to something concrete when needed? It seems very 
wrong to me to be inserting raw type casts all through this code.


It isn't entirely clear to be from a quick inspection of the code what 
the actual type usage is.  A writable general Vector should be a 
Vector and Vector just meant for reading should be a Vector 
(or the equivalent Vector).


If the type usage is "a sequence of X's and Y's" where X and Y don't 
have some useful supertype, I would recommend using a somewhat different 
set of data structures, like a list of type-safe heterogeneous 
containers or a list of a new package-level XorY class.


Buried in one of Sasha's emails it says:

"The current code uses it to store Strings and Vectors."

Hence it is declared Vector.

This is looking to me like code that should not have been generified.

David


-Joe

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

[Joe Darcy]

David Holmes wrote:

Joe Darcy said the following on 08/04/11 12:33:

David Holmes wrote:

Using wildcards makes the subtyping work along the type argument axis.


So what is the right fix here? To declare the underlying Vector as a 
Vector and cast it to something concrete when needed? It seems 
very wrong to me to be inserting raw type casts all through this code.


It isn't entirely clear to be from a quick inspection of the code 
what the actual type usage is.  A writable general Vector should be a 
Vector and Vector just meant for reading should be a 
Vector (or the equivalent Vector).


If the type usage is "a sequence of X's and Y's" where X and Y don't 
have some useful supertype, I would recommend using a somewhat 
different set of data structures, like a list of type-safe 
heterogeneous containers or a list of a new package-level XorY class.


Buried in one of Sasha's emails it says:

"The current code uses it to store Strings and Vectors."

Hence it is declared Vector.

This is looking to me like code that should not have been generified.



Hmm.  If String or Vector are the two kinds of stored values, I 
would recommend Vector> where a lone string was wrapped 
in a vector.  (Actually, I would recommend a List>, but 
that may be beyond the scope of this cleanup.


-Joe

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

[Colin Decker]

One better way to handle this in Java 8 would be to have a utility method
that takes a Supplier> SAM argument (with a no-arg method
that returns an Enumeration) and returns an Iterable that gets a new
Enumeration from the supplier each time iterator() is called. It could then
be used with method references:

Iterable iterable = Util.enumerationIterable(#object.getEnumeration);

-- 
Colin


On Wed, Aug 3, 2011 at 5:14 PM, Alexandre Boulgakov <
alexandre.boulga...@oracle.com> wrote:

> Users of Iterable expect to call Iterable.iterator() multiple times,
> receiving a fresh iterator pointing to the beginning of the Iterable each
> time. This would not be possible to do with an Enumeration wrapper since
> Enumerations are read-once.
>
> I don't know if this is a strong enough reason not to include such a
> utility class, but it could certainly be confusing.
>
> -Sasha
>
>
> On 8/3/2011 2:09 PM, Neil Richards wrote:
>
>> On Wed, 2011-08-03 at 11:03 -0700, 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.
>>>
>> Would it  be useful to have a utility object to convert an Enumeration
>> so it can be used in for-each constructs? - something like:
>> 
>> import java.util.Enumeration;
>> import java.util.Iterator;
>>
>> /**
>>  * Utility class to transform an Enumeration object such that it
>> can be used in
>>  * the for-each construct.
>>  */
>> public class IterableEnumerationHolder  implements Iterable,
>> Iterator  {
>> private final Enumeration  e;
>>
>> public IterableEnumerationHolder(**final Enumeration  e) {
>> this.e = e;
>> }
>>
>> @Override
>> public Iterator  iterator() {
>> return this;
>> }
>>
>> @Override
>> public boolean hasNext() {
>> return e.hasMoreElements();
>> }
>>
>> @Override
>> public E next() {
>> return e.nextElement();
>> }
>>
>> @Override
>> public void remove() {
>> throw new UnsupportedOperationException(**);
>> }
>> }
>> 
>>
>> If it would, perhaps it might be considered for Java 8?
>>
>> Regards,
>> Neil
>>
>>

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

[Rémi Forax]

On 08/04/2011 06:17 PM, Colin Decker wrote:

One better way to handle this in Java 8 would be to have a utility method
that takes a Supplier>  SAM argument (with a no-arg method
that returns an Enumeration) and returns an Iterable  that gets a new
Enumeration from the supplier each time iterator() is called. It could then
be used with method references:

Iterable  iterable = Util.enumerationIterable(#object.getEnumeration);



you means Collections.list().

Rémi

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

[Colin Decker]

No, that copies the Enumeration. I'm talking about something that creates
lazy Iterators backed by Enumerations.

-- 
Colin


On Thu, Aug 4, 2011 at 12:34 PM, Rémi Forax  wrote:

> On 08/04/2011 06:17 PM, Colin Decker wrote:
>
>> One better way to handle this in Java 8 would be to have a utility method
>> that takes a Supplier>  SAM argument (with a no-arg method
>> that returns an Enumeration) and returns an Iterable  that gets a
>> new
>> Enumeration from the supplier each time iterator() is called. It could
>> then
>> be used with method references:
>>
>> Iterable  iterable = Util.enumerationIterable(#**
>> object.getEnumeration);
>>
>>
> you means Collections.list().
>
> Rémi
>
>

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

[Rémi Forax]

On 08/04/2011 06:52 PM, Colin Decker wrote:
No, that copies the Enumeration. I'm talking about something that 
creates lazy Iterators backed by Enumerations.


--
Colin


Ok,
why not adding a method iterator(Enumeration) that takes an Enumeration and
returns an Iterator and then do a method reference on the method iterator.

Iterable iterable = #Collections.iterator(enumeration).iterator;

Rémi




On Thu, Aug 4, 2011 at 12:34 PM, Rémi Forax > wrote:


On 08/04/2011 06:17 PM, Colin Decker wrote:

One better way to handle this in Java 8 would be to have a
utility method
that takes a Supplier>  SAM argument (with a
no-arg method
that returns an Enumeration) and returns an Iterable
 that gets a new
Enumeration from the supplier each time iterator() is called.
It could then
be used with method references:

Iterable  iterable =
Util.enumerationIterable(#object.getEnumeration);


you means Collections.list().

Rémi

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

[Colin Decker]

Well, Iterator doesn't have an iterator() method. It also looks like you'd
have to have a reference to a single Enumeration already there? I was
suggesting using a method reference to an Enumeration-returning method so
that a fresh Enumeration could be retrieved to back each Iterator created by
the resulting Iterable. There may be a better way than I suggested... I just
wanted to point out that there's no good reason to create a single-use
Iterable for Enumerations when method references could make it easy to
create a proper Iterable.

-- 
Colin


On Thu, Aug 4, 2011 at 1:03 PM, Rémi Forax  wrote:

> **
> On 08/04/2011 06:52 PM, Colin Decker wrote:
>
> No, that copies the Enumeration. I'm talking about something that creates
> lazy Iterators backed by Enumerations.
>
> --
> Colin
>
>
> Ok,
> why not adding a method iterator(Enumeration) that takes an Enumeration and
> returns an Iterator and then do a method reference on the method iterator.
>
> Iterable iterable = #Collections.iterator(enumeration).iterator;
>
> Rémi
>
>
>
>
> On Thu, Aug 4, 2011 at 12:34 PM, Rémi Forax  wrote:
>
>> On 08/04/2011 06:17 PM, Colin Decker wrote:
>>
>>> One better way to handle this in Java 8 would be to have a utility method
>>> that takes a Supplier>  SAM argument (with a no-arg method
>>> that returns an Enumeration) and returns an Iterable  that gets a
>>> new
>>> Enumeration from the supplier each time iterator() is called. It could
>>> then
>>> be used with method references:
>>>
>>> Iterable  iterable = Util.enumerationIterable(#object.getEnumeration);
>>>
>>>
>>  you means Collections.list().
>>
>> Rémi
>>
>>
>
>

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

[Rémi Forax]

On 08/04/2011 07:39 PM, Colin Decker wrote:
Well, Iterator doesn't have an iterator() method. It also looks like 
you'd have to have a reference to a single Enumeration already there? 
I was suggesting using a method reference to an Enumeration-returning 
method so that a fresh Enumeration could be retrieved to back each 
Iterator created by the resulting Iterable. There may be a better way 
than I suggested... I just wanted to point out that there's no good 
reason to create a single-use Iterable for Enumerations when method 
references could make it easy to create a proper Iterable.


--
Colin


doh, sorry, right.

Rémi




On Thu, Aug 4, 2011 at 1:03 PM, Rémi Forax > wrote:


On 08/04/2011 06:52 PM, Colin Decker wrote:

No, that copies the Enumeration. I'm talking about something that
creates lazy Iterators backed by Enumerations.

-- 
Colin


Ok,
why not adding a method iterator(Enumeration) that takes an
Enumeration and
returns an Iterator and then do a method reference on the method
iterator.

Iterable iterable =
#Collections.iterator(enumeration).iterator;

Rémi





On Thu, Aug 4, 2011 at 12:34 PM, Rémi Forax mailto:fo...@univ-mlv.fr>> wrote:

On 08/04/2011 06:17 PM, Colin Decker wrote:

One better way to handle this in Java 8 would be to have
a utility method
that takes a Supplier>  SAM argument (with
a no-arg method
that returns an Enumeration) and returns an
Iterable  that gets a new
Enumeration from the supplier each time iterator() is
called. It could then
be used with method references:

Iterable  iterable =
Util.enumerationIterable(#object.getEnumeration);


you means Collections.list().

Rémi

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

[Alexandre Boulgakov]

On 8/3/2011 10:44 PM, Joe Darcy wrote:

David Holmes wrote:

Joe Darcy said the following on 08/04/11 12:33:

David Holmes wrote:
Using wildcards makes the subtyping work along the type argument 
axis.


So what is the right fix here? To declare the underlying Vector as 
a Vector and cast it to something concrete when needed? It seems 
very wrong to me to be inserting raw type casts all through this code.


It isn't entirely clear to be from a quick inspection of the code 
what the actual type usage is.  A writable general Vector should be 
a Vector and Vector just meant for reading should be a 
Vector (or the equivalent Vector).


If the type usage is "a sequence of X's and Y's" where X and Y don't 
have some useful supertype, I would recommend using a somewhat 
different set of data structures, like a list of type-safe 
heterogeneous containers or a list of a new package-level XorY class.


Buried in one of Sasha's emails it says:

"The current code uses it to store Strings and Vectors."

Hence it is declared Vector.

This is looking to me like code that should not have been generified.



Hmm.  If String or Vector are the two kinds of stored values, 
I would recommend Vector> where a lone string was 
wrapped in a vector.  (Actually, I would recommend a 
List>, but that may be beyond the scope of this cleanup.


I can do Vector>.

List> is probably beyond the scope of removing compiler 
warnings; getting rid of Vectors and Hashtables in general could take a 
whole summer by itself, and would probably be best to do as a whole, 
since it's not always clear at first glance if other classes depend on a 
particular object being a Vector.


-Sasha


-Joe

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

[Sebastian Sickelmann]

I like that idea. Should we open another discussion thread?


Colin Decker  schrieb:

>One better way to handle this in Java 8 would be to have a utility method
>that takes a Supplier> SAM argument (with a no-arg method
>that returns an Enumeration) and returns an Iterable that gets a new
>Enumeration from the supplier each time iterator() is called. It could then
>be used with method references:
>
>Iterable iterable = Util.enumerationIterable(#object.getEnumeration);
>
>-- 
>Colin
>
>
>On Wed, Aug 3, 2011 at 5:14 PM, Alexandre Boulgakov <
>alexandre.boulga...@oracle.com> wrote:
>
>> Users of Iterable expect to call Iterable.iterator() multiple times,
>> receiving a fresh iterator pointing to the beginning of the Iterable each
>> time. This would not be possible to do with an Enumeration wrapper since
>> Enumerations are read-once.
>>
>> I don't know if this is a strong enough reason not to include such a
>> utility class, but it could certainly be confusing.
>>
>> -Sasha
>>
>>
>> On 8/3/2011 2:09 PM, Neil Richards wrote:
>>
>>> On Wed, 2011-08-03 at 11:03 -0700, 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.

>>> Would it  be useful to have a utility object to convert an Enumeration
>>> so it can be used in for-each constructs? - something like:
>>> 
>>> import java.util.Enumeration;
>>> import java.util.Iterator;
>>>
>>> /**
>>>  * Utility class to transform an Enumeration object such that it
>>> can be used in
>>>  * the for-each construct.
>>>  */
>>> public class IterableEnumerationHolder  implements Iterable,
>>> Iterator  {
>>> private final Enumeration  e;
>>>
>>> public IterableEnumerationHolder(**final Enumeration  e) {
>>> this.e = e;
>>> }
>>>
>>> @Override
>>> public Iterator  iterator() {
>>> return this;
>>> }
>>>
>>> @Override
>>> public boolean hasNext() {
>>> return e.hasMoreElements();
>>> }
>>>
>>> @Override
>>> public E next() {
>>> return e.nextElement();
>>> }
>>>
>>> @Override
>>> public void remove() {
>>> throw new UnsupportedOperationException(**);
>>> }
>>> }
>>> 
>>>
>>> If it would, perhaps it might be considered for Java 8?
>>>
>>> Regards,
>>> Neil
>>>
>>>

-- 
Diese Nachricht wurde von meinem Android Mobiltelefon mit GMX Mail gesendet.

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

[Alexandre Boulgakov]

Here's the second version:
http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/ 



 * Changed LdapResult.referrals to be a Vector>;
 * Refactored
 o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration;
 o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames;
 o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into
   AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration;
   changed LdapBindingEnumeration and LdapSearchEnumeration
   accordingly, as well as a few of their consumers;
 * Made a few additional small changes that were discussed.

Thanks to everyone who reviewed the last version!

Thanks,
Sasha

On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:


On 8/3/2011 10:44 PM, Joe Darcy wrote:

David Holmes wrote:

Joe Darcy said the following on 08/04/11 12:33:

David Holmes wrote:
Using wildcards makes the subtyping work along the type argument 
axis.


So what is the right fix here? To declare the underlying Vector as 
a Vector and cast it to something concrete when needed? It 
seems very wrong to me to be inserting raw type casts all through 
this code.


It isn't entirely clear to be from a quick inspection of the code 
what the actual type usage is.  A writable general Vector should be 
a Vector and Vector just meant for reading should be a 
Vector (or the equivalent Vector).


If the type usage is "a sequence of X's and Y's" where X and Y 
don't have some useful supertype, I would recommend using a 
somewhat different set of data structures, like a list of type-safe 
heterogeneous containers or a list of a new package-level XorY class.


Buried in one of Sasha's emails it says:

"The current code uses it to store Strings and Vectors."

Hence it is declared Vector.

This is looking to me like code that should not have been generified.



Hmm.  If String or Vector are the two kinds of stored values, 
I would recommend Vector> where a lone string was 
wrapped in a vector.  (Actually, I would recommend a 
List>, but that may be beyond the scope of this cleanup.


I can do Vector>.

List> is probably beyond the scope of removing compiler 
warnings; getting rid of Vectors and Hashtables in general could take 
a whole summer by itself, and would probably be best to do as a whole, 
since it's not always clear at first glance if other classes depend on 
a particular object being a Vector.


-Sasha


-Joe

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

[Xuelei Fan]

On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:
> Here's the second version:
> http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/
> 
> 
>   * Changed LdapResult.referrals to be a Vector>;
>   * Refactored
>   o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration;
>   o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames;
>   o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into
> AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration;
> changed LdapBindingEnumeration and LdapSearchEnumeration
> accordingly, as well as a few of their consumers;
Is it necessary to rename LdapNamingEnumeration to
LdapNameClassPairEnumeration? I think it might not good for sustaining
work, as when backport a fix from JDK 8 to previous releases, we will
have to recognize and rename the class accordingly.

Otherwise, looks fine to me.

>   * Made a few additional small changes that were discussed.
> 
I did not review other updates. There are too many files, if you want me
review all of the changes again, please let me know.

Thanks,
Xuelei

> Thanks to everyone who reviewed the last version!
> 
> Thanks,
> Sasha
> 
> On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:
>>
>> On 8/3/2011 10:44 PM, Joe Darcy wrote:
>>> David Holmes wrote:
 Joe Darcy said the following on 08/04/11 12:33:
> David Holmes wrote:
>>> Using wildcards makes the subtyping work along the type argument
>>> axis.
>>
>> So what is the right fix here? To declare the underlying Vector as
>> a Vector and cast it to something concrete when needed? It
>> seems very wrong to me to be inserting raw type casts all through
>> this code.
>
> It isn't entirely clear to be from a quick inspection of the code
> what the actual type usage is.  A writable general Vector should be
> a Vector and Vector just meant for reading should be a
> Vector (or the equivalent Vector).
>
> If the type usage is "a sequence of X's and Y's" where X and Y
> don't have some useful supertype, I would recommend using a
> somewhat different set of data structures, like a list of type-safe
> heterogeneous containers or a list of a new package-level XorY class.

 Buried in one of Sasha's emails it says:

 "The current code uses it to store Strings and Vectors."

 Hence it is declared Vector.

 This is looking to me like code that should not have been generified.

>>>
>>> Hmm.  If String or Vector are the two kinds of stored values,
>>> I would recommend Vector> where a lone string was
>>> wrapped in a vector.  (Actually, I would recommend a
>>> List>, but that may be beyond the scope of this cleanup.
>>
>> I can do Vector>.
>>
>> List> is probably beyond the scope of removing compiler
>> warnings; getting rid of Vectors and Hashtables in general could take
>> a whole summer by itself, and would probably be best to do as a whole,
>> since it's not always clear at first glance if other classes depend on
>> a particular object being a Vector.
>>
>> -Sasha
>>>
>>> -Joe
>>>

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

[Alexandre Boulgakov]

I can change it back to LdapNamingEnumeration. I just thought it would be more 
consistent with LdapBindingEnumeration and LdapSearchEnumeration. I did not 
think of the back-porting issue. Would it be better to change 
AbstractLdapNamingEnumeration to LdapNamingEnumeration, as far as back-porting, 
since they share the most code? Or do you mean for back-porting code that uses 
these enumerations?

I don't think you need to review the other changes; they are specific change 
that were already discussed and agreed on.

Thanks,
Sasha

- Original Message -
From: xuelei@oracle.com
To: alexandre.boulga...@oracle.com
Cc: joe.da...@oracle.com, core-libs-dev@openjdk.java.net
Sent: Monday, August 8, 2011 6:58:41 PM GMT -08:00 US/Canada Pacific
Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build 
with javac -Xlint:all -Werror]

On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:
> Here's the second version:
> http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/
> <http://cr.openjdk.java.net/%7Emduigou/7072353/2/webrev/>
> 
>   * Changed LdapResult.referrals to be a Vector>;
>   * Refactored
>   o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration;
>   o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames;
>   o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into
> AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration;
> changed LdapBindingEnumeration and LdapSearchEnumeration
> accordingly, as well as a few of their consumers;
Is it necessary to rename LdapNamingEnumeration to
LdapNameClassPairEnumeration? I think it might not good for sustaining
work, as when backport a fix from JDK 8 to previous releases, we will
have to recognize and rename the class accordingly.

Otherwise, looks fine to me.

>   * Made a few additional small changes that were discussed.
> 
I did not review other updates. There are too many files, if you want me
review all of the changes again, please let me know.

Thanks,
Xuelei

> Thanks to everyone who reviewed the last version!
> 
> Thanks,
> Sasha
> 
> On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:
>>
>> On 8/3/2011 10:44 PM, Joe Darcy wrote:
>>> David Holmes wrote:
>>>> Joe Darcy said the following on 08/04/11 12:33:
>>>>> David Holmes wrote:
>>>>>>> Using wildcards makes the subtyping work along the type argument
>>>>>>> axis.
>>>>>>
>>>>>> So what is the right fix here? To declare the underlying Vector as
>>>>>> a Vector and cast it to something concrete when needed? It
>>>>>> seems very wrong to me to be inserting raw type casts all through
>>>>>> this code.
>>>>>
>>>>> It isn't entirely clear to be from a quick inspection of the code
>>>>> what the actual type usage is.  A writable general Vector should be
>>>>> a Vector and Vector just meant for reading should be a
>>>>> Vector (or the equivalent Vector).
>>>>>
>>>>> If the type usage is "a sequence of X's and Y's" where X and Y
>>>>> don't have some useful supertype, I would recommend using a
>>>>> somewhat different set of data structures, like a list of type-safe
>>>>> heterogeneous containers or a list of a new package-level XorY class.
>>>>
>>>> Buried in one of Sasha's emails it says:
>>>>
>>>> "The current code uses it to store Strings and Vectors."
>>>>
>>>> Hence it is declared Vector.
>>>>
>>>> This is looking to me like code that should not have been generified.
>>>>
>>>
>>> Hmm.  If String or Vector are the two kinds of stored values,
>>> I would recommend Vector> where a lone string was
>>> wrapped in a vector.  (Actually, I would recommend a
>>> List>, but that may be beyond the scope of this cleanup.
>>
>> I can do Vector>.
>>
>> List> is probably beyond the scope of removing compiler
>> warnings; getting rid of Vectors and Hashtables in general could take
>> a whole summer by itself, and would probably be best to do as a whole,
>> since it's not always clear at first glance if other classes depend on
>> a particular object being a Vector.
>>
>> -Sasha
>>>
>>> -Joe
>>>

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

[Xuelei Fan]

On Aug 9, 2011, at 10:16 AM, Alexandre Boulgakov 
 wrote:

> I can change it back to LdapNamingEnumeration. I just thought it would be 
> more consistent with LdapBindingEnumeration and LdapSearchEnumeration. I did 
> not think of the back-porting issue. Would it be better to change 
> AbstractLdapNamingEnumeration to LdapNamingEnumeration, as far as 
> back-porting, since they share the most code? Or do you mean for back-porting 
> code that uses these enumerations?
> 
I would prefer to change it back to LNE, and keep the new ALNE class, since the 
most confusing place is the code using the LNE, I think.

Xuelei



> I don't think you need to review the other changes; they are specific change 
> that were already discussed and agreed on.
> 
> Thanks,
> Sasha
> 
> - Original Message -
> From: xuelei@oracle.com
> To: alexandre.boulga...@oracle.com
> Cc: joe.da...@oracle.com, core-libs-dev@openjdk.java.net
> Sent: Monday, August 8, 2011 6:58:41 PM GMT -08:00 US/Canada Pacific
> Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build 
> with javac -Xlint:all -Werror]
> 
> On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:
>> Here's the second version:
>> http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/
>> <http://cr.openjdk.java.net/%7Emduigou/7072353/2/webrev/>
>> 
>>  * Changed LdapResult.referrals to be a Vector>;
>>  * Refactored
>>  o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration;
>>  o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames;
>>  o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into
>>AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration;
>>changed LdapBindingEnumeration and LdapSearchEnumeration
>>accordingly, as well as a few of their consumers;
> Is it necessary to rename LdapNamingEnumeration to
> LdapNameClassPairEnumeration? I think it might not good for sustaining
> work, as when backport a fix from JDK 8 to previous releases, we will
> have to recognize and rename the class accordingly.
> 
> Otherwise, looks fine to me.
> 
>>  * Made a few additional small changes that were discussed.
>> 
> I did not review other updates. There are too many files, if you want me
> review all of the changes again, please let me know.
> 
> Thanks,
> Xuelei
> 
>> Thanks to everyone who reviewed the last version!
>> 
>> Thanks,
>> Sasha
>> 
>> On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:
>>> 
>>> On 8/3/2011 10:44 PM, Joe Darcy wrote:
>>>> David Holmes wrote:
>>>>> Joe Darcy said the following on 08/04/11 12:33:
>>>>>> David Holmes wrote:
>>>>>>>> Using wildcards makes the subtyping work along the type argument
>>>>>>>> axis.
>>>>>>> 
>>>>>>> So what is the right fix here? To declare the underlying Vector as
>>>>>>> a Vector and cast it to something concrete when needed? It
>>>>>>> seems very wrong to me to be inserting raw type casts all through
>>>>>>> this code.
>>>>>> 
>>>>>> It isn't entirely clear to be from a quick inspection of the code
>>>>>> what the actual type usage is.  A writable general Vector should be
>>>>>> a Vector and Vector just meant for reading should be a
>>>>>> Vector (or the equivalent Vector).
>>>>>> 
>>>>>> If the type usage is "a sequence of X's and Y's" where X and Y
>>>>>> don't have some useful supertype, I would recommend using a
>>>>>> somewhat different set of data structures, like a list of type-safe
>>>>>> heterogeneous containers or a list of a new package-level XorY class.
>>>>> 
>>>>> Buried in one of Sasha's emails it says:
>>>>> 
>>>>> "The current code uses it to store Strings and Vectors."
>>>>> 
>>>>> Hence it is declared Vector.
>>>>> 
>>>>> This is looking to me like code that should not have been generified.
>>>>> 
>>>> 
>>>> Hmm.  If String or Vector are the two kinds of stored values,
>>>> I would recommend Vector> where a lone string was
>>>> wrapped in a vector.  (Actually, I would recommend a
>>>> List>, but that may be beyond the scope of this cleanup.
>>> 
>>> I can do Vector>.
>>> 
>>> List> is probably beyond the scope of removing compiler
>>> warnings; getting rid of Vectors and Hashtables in general could take
>>> a whole summer by itself, and would probably be best to do as a whole,
>>> since it's not always clear at first glance if other classes depend on
>>> a particular object being a Vector.
>>> 
>>> -Sasha
>>>> 
>>>> -Joe
>>>> 
> 

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

[Alexandre Boulgakov]

That makes sense. I'll do that with automatic refactoring in Netbeans. Do you 
want me to send another webrev afterwards?

Thanks,
Sasha

- Original Message -
From: xuelei@oracle.com
To: alexandre.boulga...@oracle.com
Cc: joe.da...@oracle.com, core-libs-dev@openjdk.java.net
Sent: Monday, August 8, 2011 7:30:09 PM GMT -08:00 US/Canada Pacific
Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build 
with javac -Xlint:all -Werror]



On Aug 9, 2011, at 10:16 AM, Alexandre Boulgakov 
 wrote:

> I can change it back to LdapNamingEnumeration. I just thought it would be 
> more consistent with LdapBindingEnumeration and LdapSearchEnumeration. I did 
> not think of the back-porting issue. Would it be better to change 
> AbstractLdapNamingEnumeration to LdapNamingEnumeration, as far as 
> back-porting, since they share the most code? Or do you mean for back-porting 
> code that uses these enumerations?
> 
I would prefer to change it back to LNE, and keep the new ALNE class, since the 
most confusing place is the code using the LNE, I think.

Xuelei



> I don't think you need to review the other changes; they are specific change 
> that were already discussed and agreed on.
> 
> Thanks,
> Sasha
> 
> - Original Message -
> From: xuelei@oracle.com
> To: alexandre.boulga...@oracle.com
> Cc: joe.da...@oracle.com, core-libs-dev@openjdk.java.net
> Sent: Monday, August 8, 2011 6:58:41 PM GMT -08:00 US/Canada Pacific
> Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build 
> with javac -Xlint:all -Werror]
> 
> On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:
>> Here's the second version:
>> http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/
>> <http://cr.openjdk.java.net/%7Emduigou/7072353/2/webrev/>
>> 
>>  * Changed LdapResult.referrals to be a Vector>;
>>  * Refactored
>>  o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration;
>>  o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames;
>>  o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into
>>AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration;
>>changed LdapBindingEnumeration and LdapSearchEnumeration
>>accordingly, as well as a few of their consumers;
> Is it necessary to rename LdapNamingEnumeration to
> LdapNameClassPairEnumeration? I think it might not good for sustaining
> work, as when backport a fix from JDK 8 to previous releases, we will
> have to recognize and rename the class accordingly.
> 
> Otherwise, looks fine to me.
> 
>>  * Made a few additional small changes that were discussed.
>> 
> I did not review other updates. There are too many files, if you want me
> review all of the changes again, please let me know.
> 
> Thanks,
> Xuelei
> 
>> Thanks to everyone who reviewed the last version!
>> 
>> Thanks,
>> Sasha
>> 
>> On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:
>>> 
>>> On 8/3/2011 10:44 PM, Joe Darcy wrote:
>>>> David Holmes wrote:
>>>>> Joe Darcy said the following on 08/04/11 12:33:
>>>>>> David Holmes wrote:
>>>>>>>> Using wildcards makes the subtyping work along the type argument
>>>>>>>> axis.
>>>>>>> 
>>>>>>> So what is the right fix here? To declare the underlying Vector as
>>>>>>> a Vector and cast it to something concrete when needed? It
>>>>>>> seems very wrong to me to be inserting raw type casts all through
>>>>>>> this code.
>>>>>> 
>>>>>> It isn't entirely clear to be from a quick inspection of the code
>>>>>> what the actual type usage is.  A writable general Vector should be
>>>>>> a Vector and Vector just meant for reading should be a
>>>>>> Vector (or the equivalent Vector).
>>>>>> 
>>>>>> If the type usage is "a sequence of X's and Y's" where X and Y
>>>>>> don't have some useful supertype, I would recommend using a
>>>>>> somewhat different set of data structures, like a list of type-safe
>>>>>> heterogeneous containers or a list of a new package-level XorY class.
>>>>> 
>>>>> Buried in one of Sasha's emails it says:
>>>>> 
>>>>> "The current code uses it to store Strings and Vectors."
>>>>> 
>>>>> Hence it is declared Vector.
>>>>> 
>>>>> This is looking to me like code that should not have been generified.
>>>>> 
>>>> 
>>>> Hmm.  If String or Vector are the two kinds of stored values,
>>>> I would recommend Vector> where a lone string was
>>>> wrapped in a vector.  (Actually, I would recommend a
>>>> List>, but that may be beyond the scope of this cleanup.
>>> 
>>> I can do Vector>.
>>> 
>>> List> is probably beyond the scope of removing compiler
>>> warnings; getting rid of Vectors and Hashtables in general could take
>>> a whole summer by itself, and would probably be best to do as a whole,
>>> since it's not always clear at first glance if other classes depend on
>>> a particular object being a Vector.
>>> 
>>> -Sasha
>>>> 
>>>> -Joe
>>>> 
> 

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

[Xuelei Fan]

On 8/9/2011 3:55 PM, Alexandre Boulgakov wrote:
> That makes sense. I'll do that with automatic refactoring in Netbeans. Do you 
> want me to send another webrev afterwards?
> 
Just to confirm, you will change LdapNameClassPairEnumeration back to
LdapNamingEnumeration, and will not change the name
AbstractLdapNamingEnumeration, right? If the answer is right, I will not
need to review the webrev any more.

Generally, I will always generate a webrev with the latest update before
push the changes. Because in the future, some others may refer to the
webrev for other work, such as researching regression failure, backport,
etc.

Thanks for your cleanup the JNDI code.

Xuelei

> Thanks,
> Sasha
> 
> - Original Message -
> From: xuelei@oracle.com
> To: alexandre.boulga...@oracle.com
> Cc: joe.da...@oracle.com, core-libs-dev@openjdk.java.net
> Sent: Monday, August 8, 2011 7:30:09 PM GMT -08:00 US/Canada Pacific
> Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build 
> with javac -Xlint:all -Werror]
> 
> 
> 
> On Aug 9, 2011, at 10:16 AM, Alexandre Boulgakov 
>  wrote:
> 
>> I can change it back to LdapNamingEnumeration. I just thought it would be 
>> more consistent with LdapBindingEnumeration and LdapSearchEnumeration. I did 
>> not think of the back-porting issue. Would it be better to change 
>> AbstractLdapNamingEnumeration to LdapNamingEnumeration, as far as 
>> back-porting, since they share the most code? Or do you mean for 
>> back-porting code that uses these enumerations?
>>
> I would prefer to change it back to LNE, and keep the new ALNE class, since 
> the most confusing place is the code using the LNE, I think.
> 
> Xuelei
> 
> 
> 
>> I don't think you need to review the other changes; they are specific change 
>> that were already discussed and agreed on.
>>
>> Thanks,
>> Sasha
>>
>> - Original Message -
>> From: xuelei@oracle.com
>> To: alexandre.boulga...@oracle.com
>> Cc: joe.da...@oracle.com, core-libs-dev@openjdk.java.net
>> Sent: Monday, August 8, 2011 6:58:41 PM GMT -08:00 US/Canada Pacific
>> Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build 
>> with javac -Xlint:all -Werror]
>>
>> On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:
>>> Here's the second version:
>>> http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/
>>> <http://cr.openjdk.java.net/%7Emduigou/7072353/2/webrev/>
>>>
>>>  * Changed LdapResult.referrals to be a Vector>;
>>>  * Refactored
>>>  o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration;
>>>  o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames;
>>>  o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into
>>>AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration;
>>>changed LdapBindingEnumeration and LdapSearchEnumeration
>>>accordingly, as well as a few of their consumers;
>> Is it necessary to rename LdapNamingEnumeration to
>> LdapNameClassPairEnumeration? I think it might not good for sustaining
>> work, as when backport a fix from JDK 8 to previous releases, we will
>> have to recognize and rename the class accordingly.
>>
>> Otherwise, looks fine to me.
>>
>>>  * Made a few additional small changes that were discussed.
>>>
>> I did not review other updates. There are too many files, if you want me
>> review all of the changes again, please let me know.
>>
>> Thanks,
>> Xuelei
>>
>>> Thanks to everyone who reviewed the last version!
>>>
>>> Thanks,
>>> Sasha
>>>
>>> On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:
>>>>
>>>> On 8/3/2011 10:44 PM, Joe Darcy wrote:
>>>>> David Holmes wrote:
>>>>>> Joe Darcy said the following on 08/04/11 12:33:
>>>>>>> David Holmes wrote:
>>>>>>>>> Using wildcards makes the subtyping work along the type argument
>>>>>>>>> axis.
>>>>>>>>
>>>>>>>> So what is the right fix here? To declare the underlying Vector as
>>>>>>>> a Vector and cast it to something concrete when needed? It
>>>>>>>> seems very wrong to me to be inserting raw type casts all through
>>>>>>>> this code.
>>>>>>>
>>>>>>> It isn't entirely clear to be from a quick inspection of the code
>>>>>>> what the actual type usage is.  A writabl

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

[Alexandre Boulgakov]

On 8/9/2011 2:53 AM, Xuelei Fan wrote:

On 8/9/2011 3:55 PM, Alexandre Boulgakov wrote:

That makes sense. I'll do that with automatic refactoring in Netbeans. Do you 
want me to send another webrev afterwards?


Just to confirm, you will change LdapNameClassPairEnumeration back to
LdapNamingEnumeration, and will not change the name
AbstractLdapNamingEnumeration, right? If the answer is right, I will not
need to review the webrev any more.

Yes, that's what I did.


Generally, I will always generate a webrev with the latest update before
push the changes. Because in the future, some others may refer to the
webrev for other work, such as researching regression failure, backport,
etc.
That makes sense. For completeness of the record here's the webrev, 
http://cr.openjdk.java.net/~mduigou/7072353/3/webrev/ 
<http://cr.openjdk.java.net/%7Emduigou/7072353/3/webrev/>.


Thanks for your cleanup the JNDI code.

Thanks for reviewing!

-Sasha


Xuelei


Thanks,
Sasha

- Original Message -
From: xuelei@oracle.com
To: alexandre.boulga...@oracle.com
Cc: joe.da...@oracle.com, core-libs-dev@openjdk.java.net
Sent: Monday, August 8, 2011 7:30:09 PM GMT -08:00 US/Canada Pacific
Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build 
with javac -Xlint:all -Werror]



On Aug 9, 2011, at 10:16 AM, Alexandre 
Boulgakov  wrote:


I can change it back to LdapNamingEnumeration. I just thought it would be more 
consistent with LdapBindingEnumeration and LdapSearchEnumeration. I did not 
think of the back-porting issue. Would it be better to change 
AbstractLdapNamingEnumeration to LdapNamingEnumeration, as far as back-porting, 
since they share the most code? Or do you mean for back-porting code that uses 
these enumerations?


I would prefer to change it back to LNE, and keep the new ALNE class, since the 
most confusing place is the code using the LNE, I think.

Xuelei




I don't think you need to review the other changes; they are specific change 
that were already discussed and agreed on.

Thanks,
Sasha

- Original Message -
From: xuelei@oracle.com
To: alexandre.boulga...@oracle.com
Cc: joe.da...@oracle.com, core-libs-dev@openjdk.java.net
Sent: Monday, August 8, 2011 6:58:41 PM GMT -08:00 US/Canada Pacific
Subject: Re: [Fwd: Code review request: 7072353 JNDI libraries do not build 
with javac -Xlint:all -Werror]

On 8/6/2011 2:11 AM, Alexandre Boulgakov wrote:

Here's the second version:
http://cr.openjdk.java.net/~mduigou/7072353/2/webrev/
<http://cr.openjdk.java.net/%7Emduigou/7072353/2/webrev/>

  * Changed LdapResult.referrals to be a Vector>;
  * Refactored
  o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration;
  o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames;
  o com/sun/jndi/ldap/*.java: refactored LdapNamingEnumeration into
AbstractLdapNamingEnumeration, LdapNameClassPairEnumeration;
changed LdapBindingEnumeration and LdapSearchEnumeration
accordingly, as well as a few of their consumers;

Is it necessary to rename LdapNamingEnumeration to
LdapNameClassPairEnumeration? I think it might not good for sustaining
work, as when backport a fix from JDK 8 to previous releases, we will
have to recognize and rename the class accordingly.

Otherwise, looks fine to me.


  * Made a few additional small changes that were discussed.


I did not review other updates. There are too many files, if you want me
review all of the changes again, please let me know.

Thanks,
Xuelei


Thanks to everyone who reviewed the last version!

Thanks,
Sasha

On 8/4/2011 2:00 PM, Alexandre Boulgakov wrote:

On 8/3/2011 10:44 PM, Joe Darcy wrote:

David Holmes wrote:

Joe Darcy said the following on 08/04/11 12:33:

David Holmes wrote:

Using wildcards makes the subtyping work along the type argument
axis.

So what is the right fix here? To declare the underlying Vector as
a Vector  and cast it to something concrete when needed? It
seems very wrong to me to be inserting raw type casts all through
this code.

It isn't entirely clear to be from a quick inspection of the code
what the actual type usage is.  A writable general Vector should be
a Vector  and Vector just meant for reading should be a
Vector  (or the equivalent Vector).

If the type usage is "a sequence of X's and Y's" where X and Y
don't have some useful supertype, I would recommend using a
somewhat different set of data structures, like a list of type-safe
heterogeneous containers or a list of a new package-level XorY class.

Buried in one of Sasha's emails it says:

"The current code uses it to store Strings and Vectors."

Hence it is declared Vector.

This is looking to me like code that should not have been generified.


Hmm.  If String or Vector  are the two kinds of stored values,
I would recommend Vector>  where a lone string was
wrapped in a vector.  (Actually, I would recommend 

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

[Dr Andrew John Hughes]

On 17:11 Tue 02 Aug , Alan Bateman wrote:
> Xuelei Fan wrote:
> > :
> > 1. I noticed the copyright date of a few files are unchanged, please
> > update them before you push the changes.
> >   
> This has come up a few times but I don't think it is strictly required. 
> Kelly or one of the release engineers run a script over the forest 
> periodically to fix up the date range. The nice thing (in my view 
> anyway) about not changing the headers is that it avoids clutter in the 
> patch and webrev. It also makes it a bit easier to transport patches to 
> other releases.
> 

I was thinking the same thing.  Copyright header changes mixed into other
patches are a nightmare as regards backporting.

> -Alan
> 

-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37