> 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” What if “paths” is non-empty but all entries are invalid and no catalog file is read - will it read the system property? line 46: s/path argument/paths argument/ Same comment applied to other factory methods. 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. Otherwise the change looks okay. Mandy
