Hi Ioi,
On 10/21/14 10:27 AM, Ioi Lam wrote:
Please review this fix:
http://cr.openjdk.java.net/~iklam/8061651-lookup-index-open-v1/
This looks better than the preliminary webrev you sent me offline
earlier. I only review the jdk repo change.
Launcher.java
line 290: it can be made a final field.
line 297: this.ucp = ....
line 317-319: I think they are meant to be two sentences.
I have the following suggested text:
// The class of the given name is not found in the parent
// class loader as well as its local URLClassPath.
// Check if this class has already been defined dynamically;
// if so, return the loaded class; otherwise, skip the parent
// delegation and findClass.
URLClassPath.java
line 76: this long line should be broken into multiple line.
Maybe you can addsun.security.action.GetPropertyAction in the
import and update line 72, 74, 76, 78.
line 319-326: Is this LoaderType enum necessary? I think it can
simply pass the ClassLoader instance to VM rather than having
this enum type just for this purpose. I suggest to get rid
of the LoaderType enum type.
line 398: what happens if loaders.size() > maxindex? Shouldn't
it return null?
404 if (getNextLoader(null, maxindex) == null ||
405 !lookupCacheEnabled) {
line 405 should indent to the right for better readability.
The comment helps as I was initially confused why lookupCacheEnabled
is not checked first. Am I right to say line 398-415 is equivalent
to (excluding the debugging statements):
if (loaders.size() <= maxindex &&getLoader(maxindex) != null) {
returnlookupCacheEnabled ? cache : null;
}
return null;
I think that is easier to understand.
432 urlNoFragString.equals(
433 URLUtil.urlNoFragString(lookupCacheURLs[index]))) {
Should you store URLUtil.urlNoFragString form of URL in
lookupCacheURLs such that they can be compared with equals?
line 423-426: formatting nit: these lines look a little long
and would be good to rewrap them.
jvm.h
1394: typo s/Retruns/Returns
As suggested above, pass the classloader instance instead of
defining a new loader_type interface
URLClassPath.c
line 42: nit: align the parameters
There are several lines calling
49JNU_ThrowNullPointerException(env, 0).
55 JNU_ThrowOutOfMemoryError(env, NULL);
The msg argument probably better to be consistent and pass NULL.
ClassLoader.c is a bit inconsistent.
Can you add regression tests in the jdk repo? Sanity tests
like with the lookup cache enabled but invalidated at startup
or due to addURL call.
Mandy
