RFR (JDK10/jaxp) 8181154: Fix lint warnings in JAXP repo: deprecation

Fri Jul 7 06:34:19 UTC 2017

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
>>
>