Thanks Mandy. Please see inline.
On 1/4/2016 6:50 PM, Mandy Chung wrote:
On Dec 23, 2015, at 2:55 PM, huizhe wang <huizhe.w...@oracle.com> wrote:
Hi,
This is an improvement to the new Catalog API on null handling. At the package
level, a null argument shall result in NPE. However, an exception was made to
the methods in CatalogManager so that they can used conveniently. With this
request, we're in favor of consistency over the minor convenience, which means
a resolver can no longer be created by:
CatalogManager.catalogResolver(null);
Instead, the following may be used to achieve the same result:
CatalogManager.catalogResolver(CatalogFeatures.defaults());
While path can be empty, it can no longer be null. The following then would
also get a NPE:
CatalogManager.catalogResolver(CatalogFeatures.defaults(), null);
This change is reasonable and it’s good to pass CatalogFeatures.defaults()
explicitly rather than special casing null to represent that.
In consistence with ignoring invalid catalogs as required by the Catalog
standard, the statements that required CatalogException when no catalog is
found are removed.
JBS: https://bugs.openjdk.java.net/browse/JDK-8144966
webrev: http://cr.openjdk.java.net/~joehw/jdk9/8144966/webrev/
41 * Creates a {@code Catalog} object using the specified feature
settings and path to
42 * a catalog file. If the path is empty,
Nit: should it say “path to one or more catalog files”
Fixed.
What if “paths” is non-empty but all entries are invalid and no catalog file is
read - will it read the system property?
It will not read the system property. The assumption is that the paths
specified are meant to be used. If they are invalid, it would indicate
programming error. I added a couple statements to make it clearer.
line 46: s/path argument/paths argument/
Same comment applied to other factory methods.
Fixed.
CatalogImpl.java
117 if (file.length > 0) {
- Do you expect file to be null? If so, NPE will be thrown. If not, file ==
null in line 125 is not needed.
removed "file == null".
Otherwise the change looks okay.
Thanks. I also added specdiff for easier reading:
JBS:https://bugs.openjdk.java.net/browse/JDK-8144966
webrev:http://cr.openjdk.java.net/~joehw/jdk9/8144966/webrev/
specdiff:
http://cr.openjdk.java.net/~joehw/jdk9/8144966/specdiff/javax/xml/catalog/package-summary.html
Joe
Mandy
