hi michael as for the other calls: i would suggest that we only have the checks in the MutableTree and not in the AbstractTree which serves as base both for the internally used ImmutableTree and the public MutableTree.
probably that's a leftover of the refactoring that resulted in the AbstractTree as for some methods that checks are in MutableTree already. so, i can instead move the checkForNull calls there, if you agree. kind regards angela On 06/11/15 11:40, "Angela Schreiber" <[email protected]> wrote: >hi michael > >but it's an internal method in an internal class and i don't see >how an API consumer can overwrite it... that's why the check really >looks redundant to me in particular as it would equally result in an NPE, >which looks bad anyway. > >kind regards >angela > >On 06/11/15 11:31, "Michael Dürig" <[email protected]> wrote: > >> >>Hi, >> >>On 6.11.15 10:03 , [email protected] wrote: >>> Author: angela >>> Date: Fri Nov 6 10:03:03 2015 >>> New Revision: 1712931 >>> >>> URL: http://svn.apache.org/viewvc?rev=1712931&view=rev >>> Log: >>> OAK-3593 : improvements to plugins/tree >>> >> >> >>> Modified: >>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/pl >>>u >>>gins/tree/impl/AbstractTree.java >>> URL: >>>http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java >>>/ >>>org/apache/jackrabbit/oak/plugins/tree/impl/AbstractTree.java?rev=171293 >>>1 >>>&r1=1712930&r2=1712931&view=diff >>> >>>======================================================================== >>>= >>>===== >>> --- >>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/pl >>>u >>>gins/tree/impl/AbstractTree.java (original) >>> +++ >>>jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/pl >>>u >>>gins/tree/impl/AbstractTree.java Fri Nov 6 10:03:03 2015 >>> @@ -18,7 +18,6 @@ >> >>> protected void buildPath(@Nonnull StringBuilder sb) { >>> AbstractTree parent = getParentOrNull(); >>> if (parent != null) { >>> - parent.buildPath(checkNotNull(sb)); >>> + parent.buildPath(sb); >>> sb.append('/').append(getName()); >>> } >> >>I don't think those null check are redundant here and I would like to >>have them back. The @Nonnull annotation does some rather week static >>analysis, which is helpful but by no means complete. The missing null >>check easily makes a API usage error look like an implementation error >>thus causing bogus bug reports. >> >>Michael >
