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:19:41 GMT

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.

> > > Sorry, I've not got time to investigate this further right now
> > > but rest assured we will get to the bottom of it and fix it
> > > correctly. Raise a JIRA if you want to make sure it doesn't get
> > > forgotten.
> >
> > Sounds good.
>
> I've reopened (but removed the must-fix-for-6.0M1 status) the JIRA at:
> 
>   https://issues.apache.org/jira/browse/HARMONY-6448
> 
> to track this issue.

Closed again as you opened HARMONY-6451 to track it.

I'll update this thread and the JIRA when I've fixed it.

Regards,
-Mark.



Mime
View raw message