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<alexandre.boulga...@oracle.com> 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<Vector<String>>;
* Refactored
o com/sun/jndi/dns/DnsContext.java: BaseNameClassPairEnumeration<T>;
o com/sun/jndi/toolkit/dir/HierMemDirCtx.java: BaseFlatNames<T>;
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<Object> and Vector just meant for reading should be a
Vector<?> (or the equivalent Vector<? extends Object>).
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 Vector<String>s."
Hence it is declared Vector<Object>.
This is looking to me like code that should not have been generified.
Hmm. If String or Vector<String> are the two kinds of stored values,
I would recommend Vector<Vector<String>> where a lone string was
wrapped in a vector. (Actually, I would recommend a
List<List<String>>, but that may be beyond the scope of this cleanup.
I can do Vector<Vector<String>>.
List<List<String>> 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
