Hi Joe,
Looks good to me. I have two additional comments, one nit - and
one for some later follow-up - so no need for a new webrev.
1. Nit: I think it should be final as well:
+++
new/src/java.xml/share/classes/com/sun/org/apache/xml/internal/serializer/OutputPropertiesFactory.java
2017-07-07 21:15:14.418212557 -0700
[...]
- private static Integer m_synch_object = new Integer(1);
+ private static Object m_synch_object = new Object();
2. Can you also add the following items to the list of things
that should be investigated in a followup/cleanup?
It bothers me that a non-deprecated method still needs to call
a deprecated API:
+++
new/src/java.xml/share/classes/com/sun/org/apache/xerces/internal/dom/PSVIElementNSImpl.java
2017-07-07 21:15:01.881586697 -0700
[...]
+ @SuppressWarnings("deprecation")
public String getSchemaDefault() {
return fDeclaration == null ? null :
fDeclaration.getConstraintValue();
}
+++
new/src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/AttributePSVImpl.java
2017-07-07 21:15:04.749729882 -0700
[...]
+ @SuppressWarnings("deprecation")
public String getSchemaDefault() {
return fDeclaration == null ? null :
fDeclaration.getConstraintValue();
}
+++
new/src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/AttributePSVImpl.java
2017-07-07 21:15:04.749729882 -0700
[...]
+ @SuppressWarnings("deprecation")
public String getSchemaDefault() {
return fDeclaration == null ? null :
fDeclaration.getConstraintValue();
}
+++
new/src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/ElementPSVImpl.java
2017-07-07 21:15:05.077746257 -0700
[...]
+ @SuppressWarnings("deprecation")
public String getSchemaDefault() {
return fDeclaration == null ? null :
fDeclaration.getConstraintValue();
}
cheers,
-- daniel
On 08/07/2017 01:53, huizhe wang wrote:
Here's the updated webrev:
http://cr.openjdk.java.net/~joehw/jdk10/8181154/webrev01/
Thanks,
Joe
On 7/7/2017 10:59 AM, huizhe wang wrote:
#11
com/sun/org/apache/xerces/internal/jaxp/datatype/XMLGregorianCalendarImpl.java
1166 final public void setYear(BigInteger year) {
nit: for consistency you should reorder the keywords to:
public final void
same file:
- BigInteger.valueOf((long) 1) => BigInteger.ONE
- new BigDecimal(TWELVE) => introduce a constant DECIMAL_TWELVE?
- new BigDecimal(TWENTY_FOUR) => introduce a constant
DECIMAL_TWENTY_FOUR?
Added the two constants.
#12
com/sun/org/apache/xerces/internal/parsers/AbstractSAXParser.java
81 @SuppressWarnings("deprecation")
82 public abstract class AbstractSAXParser
83 extends AbstractXMLDocumentParser
84 implements PSVIProvider, // PSVI
85 Parser, XMLReader // SAX1, SAX2
A better solution would be to deprecate the methods that implement
deprecated methods - and avoid using deprecated APIs elsewhere...
#13
com/sun/org/apache/xerces/internal/util/AttributesProxy.java
36 @SuppressWarnings("deprecation")
37 public final class AttributesProxy
38 implements AttributeList, Attributes2 {
I wonder whether deprecating the methods implementing a deprecated
API would allow to remove @SuppressWarnings("deprecation") - maybe not
in this case since this class needs to implement a deprecated interface
and a non deprecated one (and therefore the class itself can't be
deprecated). Anyway it might be good to add the @Deprecated
annotations to deprecated methods.
We'll take a closer look at this through [1], for both #12 and #13,
and #17 as well. AttributesProxy is used quite widely. We'll need to
get into the details to see if we can remove the deprecated
AttributeList.
Since there are import references that are deprecated, the suppress
annotation has to be added at the class.
[1] https://bugs.openjdk.java.net/browse/JDK-8183972
#14
com/sun/org/apache/xml/internal/dtm/ref/IncrementalSAXSource_Xerces.java
if _main use deprecated APIs then maybe _main should be @Deprecated
as well.
Done.
#15
com/sun/org/apache/xml/internal/serializer/OutputPropertiesFactory.java
207 private static Integer m_synch_object = 1;
This is not correct! Since this object is only use as a synchronization
lock then please change this to:
private static Object m_synch_object = new Object();
Ok, fixed now. But I don't think the sync is necessary, logged
https://bugs.openjdk.java.net/browse/JDK-8184019 to take a look later
and can also do some cleanups.
#16
com/sun/org/apache/xpath/internal/axes/HasPositionalPredChecker.java
Can this class be changed to stop using deprecated APIs instead?
It can probably be removed, will do so through
https://bugs.openjdk.java.net/browse/JDK-8184020.
#17
javax/xml/parsers/SAXParser.java
org/xml/sax/helpers/ParserAdapter.java
org/xml/sax/helpers/XMLReaderAdapter.java
Can these classes be changed to stop using deprecated APIs instead?
Or at least a cleanup issue logged for that effect?
Most likely we can't since these are public APIs. DocumentHandler for
example, is still usable. We can probably add forRemoval and then
remove in the next release.
That's all!
Thanks for all of that!
Best,
Joe
best regards,
-- daniel
On 05/07/2017 19:48, Joe Wang wrote:
Hi,
This patch fixes deprecation warnings in the jaxp repo. I'm
limiting the changes to deprecation or the 102 classes. The
cup-generated classes, XPath*.java, could use some cleanup or even
rewrite, but that will be left in future works (JDK-8183580).
Most of the changes are straight-forward, but I did have trouble
with the replacing clazz.newInstance() with
clazz.getDeclaredConstructor().newInstance() since in some cases I
got RuntimePermission problems. So for most of such cases, I've
replaced them with getConstructor() instead, thanks Alan for the
suggestion. For specific cases such as xsltc/compiler/Parser.java,
I'm using getDeclaredConstructor() because some of the classes
didn't have a public constructor.
Again, please bear with me for the cup-generated classes that have
super long lines (e.g. XPathParser.java). Please read with Udiffs
instead of Sdiffs.
Some of the boxing (e.g. Integer.valueOf()) in the BCEL classes may
not be necessary, but that's how they look in BCEL to which an
update is scheduled in JDK 10.
All tests passed.
JBS: https://bugs.openjdk.java.net/browse/JDK-8181154
webrevs: http://cr.openjdk.java.net/~joehw/jdk10/8181154/webrev/
Thanks
Joe
