On 7/6/17, 4:03 AM, Daniel Fuchs wrote:
Hi Joe,
Waouh, lost of good cleanup! A few comments:
Thanks Daniel! That's a lot of good comments.
#1:
com/sun/org/apache/xalan/internal/xsltc/dom/NodeSortRecordFactory.java:
154 sortRecord =
(NodeSortRecord)_class.getConstructor().newInstance();
Maybe it would be good to add a comment to say that NodeSortRecord
subclasses are generated with a public empty constructor (which is
if I'm not mistaken, what
com.sun.org.apache.xalan.internal.xsltc.compiler.Sort::compileInit
does).
Note added.
#2:
com/sun/org/apache/xalan/internal/xsltc/runtime/Attributes.java:
30 @SuppressWarnings("deprecation")
31 public final class Attributes implements AttributeList {
I wonder if the correct thing to do here would be to deprecate this
class, since it doesn't seem to be anything but the implementation
of a deprecated interface. Or alternatively, change it to implement
the org.xml.sax.Attributes interface?
Maybe the @SuppresWarnings can be kept as a stop gap solution for
now, and a new issue logged to investigate what's the proper thing
to do - as I suspect the answer might not be straightforward...
Sounds good, logged: https://bugs.openjdk.java.net/browse/JDK-8183972
It covers #2, #3, #10, #12, #13, #17.
#3:
com/sun/org/apache/xalan/internal/xsltc/trax/TrAXFilter.java
com/sun/org/apache/xalan/internal/xsltc/trax/Util.java
com/sun/org/apache/xerces/internal/impl/xs/traversers/XSDHandler.java
com/sun/org/apache/xpath/internal/SourceTreeManager.java
51 @SuppressWarnings("deprecation")
//org.xml.sax.helpers.XMLReaderFactory
Same here: maybe we should log a further issue to investigate
whether javax.xml.parsers.SAXParserFactory could be used instead
of the deprecated factory in these classes?
#4:
com/sun/org/apache/xerces/internal/dom/CoreDOMImplementationImpl.java
Is this fixing a different issue?
The try-catch only applies to the use of Apache standalone. In the JDK,
there's no need for it (fall back to the deprecated one).
#5:
com/sun/org/apache/xerces/internal/dom/PSVIAttrNSImpl.java
com/sun/org/apache/xerces/internal/dom/PSVIElementNSImpl.java
com/sun/org/apache/xerces/internal/impl/xs/AttributePSVImpl.java
com/sun/org/apache/xerces/internal/impl/xs/ElementPSVImpl.java
114 @SuppressWarnings("deprecation")
115 public String getSchemaDefault() {
Use fDeclaration.getValueConstraintValue().getNormalizedValue() instead.
That was suggested when it was deprecated. However, as you can see the
implementer never followed it through. First of all, the impl of
getConstraintValue has an additional type check and then stringValue
checks if there's actualValue before returning normalizedValue, while
the above methods always return normalizedValue. This is different from
ItemPSVI where deprecated method such as getActualNormalizedValue can
indeed be replaced, as suggested, by getSchemaValue().getActualValue().
126 @SuppressWarnings("deprecation")
127 public String getSchemaNormalizedValue() {
245 @SuppressWarnings("deprecation")
246 public Object getActualNormalizedValue() {
253 @SuppressWarnings("deprecation")
254 public short getActualNormalizedValueType() {
261 @SuppressWarnings("deprecation")
262 public ShortList getItemValueTypes() {
I believe the correct thing to do here instead would be to deprecate
these methods - that should get rid of the warnings.
True. My script only knows to add the suppress annotation :-). They can
be removed too since they are internal APIs.
#6
com/sun/org/apache/xerces/internal/impl/XMLEntityManager.java
com/sun/org/apache/xerces/internal/impl/XMLEntityScanner.java
An alternative solution would be to use Boolean.TRUE and Boolean.FALSE.
Not sure if it matters though. That was just a thought.
True, but it won't since it casts to Boolean. The whole process can
probably be improved with a static encoding map, but it's not worth the
effort. I initially simply want to get rid of warnings, which is why my
script just does the unboxing, or in the above cases adds suppress
annotation. Many improvement can be made to the old code, but if it
doesn't add significant benefit, for example, improving performance,
then I would say it's not worth it since we could spend the time on many
other things.
#7
com/sun/org/apache/xerces/internal/impl/dtd/models/CMLeaf.java
Not sure why you converted StringBuffer to StringBuilder in the
previous file but not there.
Ah, didn't see it, replaced it now. If you search the code, you'll get
over 1000 matches. So I made the change when I saw it. Too bad I did a
full update on the obsolete lang features for the standalone, but it was
not carried over.
#8
com/sun/org/apache/xerces/internal/impl/xs/XSAttributeDecl.java
com/sun/org/apache/xerces/internal/impl/xs/XSAttributeUseImpl.java
com/sun/org/apache/xerces/internal/impl/xs/XSElementDecl.java
161 @SuppressWarnings("deprecation")
162 public String getConstraintValue() {
198 @SuppressWarnings("deprecation")
199 public Object getActualVC() {
205 @SuppressWarnings("deprecation")
206 public short getActualVCType() {
212 @SuppressWarnings("deprecation")
213 public ShortList getItemValueTypes() {
These methods should be marked deprecated since they implement
deprecated methods. If I'm not mistaken then this should allow
you to remove the @SuppressWarnings("deprecation") from their
declaration.
Done.
#9
com/sun/org/apache/xerces/internal/impl/xs/traversers/XSDComplexTypeTraverser.java
- fGlobalStore[fGlobalStorePos++] = new Integer((fDerivedBy <<
16) + fFinal);
- fGlobalStore[fGlobalStorePos++] = new Integer((fBlock << 16)
+ fContentType);
+ fGlobalStore[fGlobalStorePos++] = (fDerivedBy << 16) + fFinal;
+ fGlobalStore[fGlobalStorePos++] = (fBlock << 16) + fContentType;
I wonder if there can be some subtle type conversion side effects here,
given that all the operands are shorts, and that we expect an int in
the end?
These operations resulted in an Integer. They are used that way too
(case to Integer).
#10
com/sun/org/apache/xerces/internal/jaxp/SAXParserImpl.java
66 @SuppressWarnings("deprecation")
Why is this needed?
#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?
Reasonable. Will do this and continue on the rest tomorrow.
Thanks,
Joe
#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.
#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.
#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();
#16
com/sun/org/apache/xpath/internal/axes/HasPositionalPredChecker.java
Can this class be changed to stop using deprecated APIs instead?
#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?
That's all!
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
