Thanks Daniel!
On 5/9/2016 4:05 AM, Daniel Fuchs wrote:
Hi Joe,
This looks much better to me.
I just wonder about the @Deprecated("5") vs @Deprecated("1.5")
Yes, the compiler accepted it just fine and produced javadocs similar to
that of XMLReaderFactory where since="9".
Best,
Joe
We started dropping the 1. for Java 5 - so I guess
@Deprecated("5") is OK. Hopefully Joe Darcy will be able
to confirm :-)
best regards,
-- daniel
On 04/05/16 02:22, huizhe wang wrote:
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
