In message <20100217192622.3b6b8478...@athena.apache.org>, Mark Hindess writes: > > In message <a43fbc6a1002171030p15b49041pee64ed782ad56...@mail.gmail.com>, > Jesse Wilson writes: > > > > On Wed, Feb 17, 2010 at 9:03 AM, Mark Hindess > > <mark.hind...@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.
