On 2/18/2016 5:15 AM, Langer, Christoph wrote:
Hi Joe,

here is the next version:
http://cr.openjdk.java.net/~clanger/webrevs/8149915.2/

Looks good!  Thanks.

Copyright year in XSAttributeChecker.java is still incorrect. I meant to say the year "2016" needs to have a comma, so in this case, it needs to be "2015, 2016, " rather than "2015, 2016".

Best,
Joe


Hopefully the final one :-)

Yes, it's good to have some cleanups. For the generic initialization,
it's preferable to have no redundant type arguments, for example,
instead of:

protected Stack<Entity> fEntityStack = new Stack<Entity>();

do:
protected Stack<Entity> fEntityStack = new Stack<>();

Some of the "cleanup" in the webrev were reversing <> with added type
argument(s), that would be unnecessary.
I think I did that on all the places that I touched now.

For the obsolete Vector, it would be good to replace it with ArrayList.
There are so many Vector instances in this code that it's out of scope for me 
now to touch them all...

Copyright year "2016" in XSAttributeChecker needs to be "2016," or else
the script would complain.
For that one I thought that it would look like "2015,2016" if the initial 
copyright was done in 2015. But I changed it as you suggested.

In XSDHandler, note that there's another call to get SECURITY_MANAGER
towards the end of the reset method (at 3572 in your new version) that
may be removed. You may also consolidate most of those
setFeature/Property statements in one try-catch block if you want.
OK, I did some further refacturing here. However, I've left the try/catch 
blocks as I think it's the intention to silently catch some exceptions but then 
try to go on with the next property.

For the test, the @bug tag is still desirable in the general comment
section. For now, it's @bug 8149915. As we add more test to the class in
the future, we'd then append more bug ids, @bug 8149915, xxxxxxx.
Done. I also left the @bug in the comments for the test method.

I'm also wondering why some files have a copyright header like this, e.g.
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/XML11D
TDScannerImpl.java:
/*
   * reserved comment block
   * DO NOT REMOVE OR ALTER!
   */

Shouldn't there be the standard copyright header or is there some reason
behind it?

The block is used by the licensing script to swap with license
information required by licensees. For code that came from Xerces, we
should keep the block as is with the Apache License. But when we make
changes, we're required to put in the copyright header. Since the
changes you made are cleanup / removing unused fields, it's okay to keep
the block as is.
OK :)

Thanks again for reviewing.

All the Best
Christoph



Reply via email to