On 5/3/2016 3:36 AM, Daniel Fuchs wrote:
Hi Joe,
This look good but the implementation might be overly complex,
which makes it difficult to read.
It was basically the existing code with some cleanup. What's in
jarLookup was a copy of the original code. As you can see I was eager to
add ServiceLoader support and forget about it (deprecated) :-)
First:
141 ClassLoader cl = ss.getContextClassLoader();
is misnamed, because as far as I can see this method returns
the context class loader if not null, otherwise the system
class loader.
Make sense. That method was changed. I now renamed it to getClassLoader
with javadoc.
So cl is either the context class loader or the system class
loader and is guaranteed to be non null.
I would suggest adding a comment to make it clear.
This means in turn that the new jarLookup(ClassLoader)
method can be greatly simplified - you can do:
final ClassLoader cl = Objects.requiresNonNull(loader);
and then simplify the if (cl != null) else ... logic,
because you now know that cl cannot be null.
The cl-null checks in both jarLookup and findServiceProvider are removed.
The new webrev also includes some cleanup of warnings, deprecation with
javadocs (since Java SE 5) but without the annotation. I limited the
patch to the sax package only.
New webrev:
http://cr.openjdk.java.net/~joehw/jdk9/8152912/webrev01/
Best,
Joe
Best regards,
-- daniel
On 02/05/16 20:30, huizhe wang wrote:
Hi,
Please review a change that adds a step using the ServiceLoader to
XMLReaderFactory's provider-lookup process. Meanwhile, XMLReaderFactory
is deprecated in favor of javax.xml.parsers.SAXParserFactory.
JBS: https://bugs.openjdk.java.net/browse/JDK-8152912
Webrev: http://cr.openjdk.java.net/~joehw/jdk9/8152912/webrev/
The change has been verified by SQE test that Frank will submit soon for
review (that is, when he starts to work at his local time).
Thanks,
Joe
