You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@harmony.apache.org by Jesse Wilson <je...@google.com> on 2010/02/17 17:35:10 UTC
Re: svn commit: r910980 - /harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/util/TreeMap.java
I noticed something weird in commit 910980:
Author: hindessm
Date: Wed Feb 17 14:07:42 2010
New Revision: 910980
URL: http://svn.apache.org/viewvc?rev=910980&view=rev
Log:
Thought I'd reverted this already ... reverting r903454. See:
http://markmail.org/thread/cdxlmi26mjgor3om
Modified:
harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/util/TreeMap.java
I think you're moving in the wrong direction. The toComparable method needs
a null check. You can see this by reading the TreeMap code from the Java 5
branch<http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/luni/src/main/java/java/util/TreeMap.java>.
Harmony doesn't have explicit test coverage here, but you can verify the bug
by running other publicly-available JDK test suites against Harmony.
Mark, can you revert this commit?
Thanks,
Jesse
Re: svn commit: r910980 - /harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/util/TreeMap.java
Posted by Mark Hindess <ma...@googlemail.com>.
In message <20...@d06av01.portsmouth.uk.ibm.com>,
Mark Hindess writes:
>
>
> In message <20...@athena.apache.org>, Mark
> Hindess writes:
>
> > In message
> > <a4...@mail.gmail.com>, Jesse
> > Wilson writes:
> >
> > > On Wed, Feb 17, 2010 at 9:03 AM, Mark Hindess
> > > <ma...@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.
Re: svn commit: r910980 - /harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/util/TreeMap.java
Posted by Mark Hindess <ma...@googlemail.com>.
In message <20...@athena.apache.org>, Mark Hindess writes:
>
> In message <a4...@mail.gmail.com>,
> Jesse Wilson writes:
> >
> > On Wed, Feb 17, 2010 at 9:03 AM, Mark Hindess
> > <ma...@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.
Re: svn commit: r910980 - /harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/util/TreeMap.java
Posted by Mark Hindess <ma...@googlemail.com>.
In message <a4...@mail.gmail.com>,
Jesse Wilson writes:
>
> On Wed, Feb 17, 2010 at 9:03 AM, Mark Hindess
> <ma...@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.
> > 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.
> In general I don't think Harmony developers should submit changes when
> they don't have time to investigate the problems that result from
> them. Ie. if a change is controversial, the burden should be on the
> submitter!
Agreed. In this case, I'm a little curious and keen to improve our
test coverage so I am prepared to invest some time on it.
Regards,
Mark.
Re: svn commit: r910980 - /harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/util/TreeMap.java
Posted by Jesse Wilson <je...@google.com>.
On Wed, Feb 17, 2010 at 9:03 AM, Mark Hindess
<ma...@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.
> 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.
In general I don't think Harmony developers should submit changes when they
don't have time to investigate the problems that result from them. Ie. if a
change is controversial, the burden should be on the submitter!
Re: svn commit: r910980 - /harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/util/TreeMap.java
Posted by Mark Hindess <ma...@googlemail.com>.
In message <a4...@mail.gmail.com>,
Jesse Wilson writes:
>
> I noticed something weird in commit 910980:
>
> Author: hindessm
> Date: Wed Feb 17 14:07:42 2010
> New Revision: 910980
>
> URL: http://svn.apache.org/viewvc?rev=910980&view=rev
> Log:
> Thought I'd reverted this already ... reverting r903454. See:
>
> http://markmail.org/thread/cdxlmi26mjgor3om
>
> Modified:
> harmony/enhanced/classlib/branches/java6/modules/luni/src/main/java/java/
> util/TreeMap.java
>
> I think you're moving in the wrong direction. The toComparable method
> needs a null check. You can see this by reading the TreeMap code from
> the Java 5
> branch<http://svn.apache.org/viewvc/harmony/enhanced/classlib/trunk/modules/l
> uni/src/main/java/java/util/TreeMap.java>.
> Harmony doesn't have explicit test coverage here, but you can verify
> the bug by running other publicly-available JDK test suites against
> Harmony.
>
> Mark, can you revert this commit?
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.
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.
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.
Regards,
Mark.