harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Hindess <mark.hind...@googlemail.com>
Subject Re: svn commit: r910980 - /harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/util/TreeMap.java
Date Wed, 17 Feb 2010 20:38:30 GMT

In message <201002172019.o1HKJf33002559@d06av01.portsmouth.uk.ibm.com>,
Mark Hindess writes:
>
> 
> In message <20100217192622.3B6B847841B@athena.apache.org>, Mark
> Hindess writes:
>
> > In message
> > <a43fbc6a1002171030p15b49041pee64ed782ad56c67@mail.gmail.com>, Jesse
> > Wilson writes:
> >
> > > On Wed, Feb 17, 2010 at 9:03 AM, Mark Hindess
> > > <mark.hindess@googlemail.com> wrote:
> > > 
> > > > I have solid evidence that this change is wrong... it breaks
> > > > Harmony tests that pass on the RI.  I've no solid evidence that
> > > > this change is required.  So my inclination is not to put the
> > > > check back.
> > >
> > > That's curious, but I guess I haven't looked into the navigable
> > > stuff that's new in 1.6.
> > > 
> > > > A null check may be required but I'm not convinced this is the
> > > > right place for it.  The toComparable method is just a trivial
> > > > helper function that is clearly being used (probably quite
> > > > reasonably) in a context that doesn't require a null check.
> > > 
> > > I think toComparable() is the right place. TreeMaps operates in
> > > two different modes:
> > > 
> > >    - *With a comparator.* In this mode objects don't need to be
> > >    comparable.  Null is permitted, because comparators can decide
> > >    where null fits in the total ordering.
> > 
> > >    - *Without a comparator.* In this mode everything must be
> > >    comparable.  Null is not comparable and must be forbidden.
> > 
> > That was my understanding too but I'm assuming it is not that
> > simple.  I suppose the other option is that toComparable is being
> > called when it should not be.
>
> Turns out it is simply a problem with the ordering of the checks for:
> 
> a) null keys
> b) empty maps
> 
> For several method the RI obviously checks if a map is empty and returns
> null with no exception regardless of the key where as with your fix the
> null key causes an exception first.  A number of methods are protected
> with an isEmpty check - root == null - that short circuits the full
> traversal but some are not.  I'll fix them shortly and re-instate your
> fix.

Fixed in r911169.

-Mark.



Mime
View raw message