Hi Jason. I'm the engineer implementing this RFE. Mike is committing
the fix for me as I don't yet have commit rights to the repository.
Please see my comments inline.
Darryl
-------- Original Message --------
Subject: RE: Code Review Request for 4533691 (add
Collections.EMPTY_SORTED_SET)
Date: Fri, 4 Nov 2011 12:52:54 -0500
From: Jason Mehrens <jason_mehr...@hotmail.com>
To: <mike.dui...@oracle.com>, <david.hol...@oracle.com>
CC: core-libs-dev@openjdk.java.net
Mike,
These 'simple' classes are really hard to get right. Here is my review of the
change:
1. Why not extend EmptySet? You wouldn't have to implement so many methods.
Good point.
2. The comparator method is using raw types.
The SortedSet.comparator() method spec allows returning of null.
3. The readResolve method comment is just wrong. Create a default access
static final reference named EMPTY_SORTED_SET inside of the EmptySortedSet and
use that in readResolve and in Collections.emptySortedSet.
Yes. This comment was left over from when the EMPTY_SORTED_SET field was
there. I missed this, thanks.
4. Only the IAE if statement is need for your range checks. NPE and CCE will
occur in that if statement by default. CCE lacks a descriptive message the you
get if you used Class.cast or just an implicit cast.
True. I'm trying to be clear in the code that the parameters are checked for
these things. Is this an optimization/style issue? What is the preference
here?
5. What if I want to create an empty set sorted set with supplied comparator?
Extend EmptySortedSet and override the comparator method. I believe most uses
of EmptySortedSet will not want to supply their own Comparator. I can support
a suppliable Comparator if there's enough interest.
6. Why not implement an EmptyNavigableSet instead since the bug was entered
before 1.6?
Is NavigableSet preferrable to SortedSet? There is currently no request for
EmptyNavigableSet, just EmptySortedSet.
Did I make the case for a follow up review? :)
Regards,
Jason
Subject: Re: Code Review Request for 4533691 (add Collections.EMPTY_SORTED_SET)
From:mike.dui...@oracle.com
Date: Fri, 4 Nov 2011 09:29:09 -0700
To:david.hol...@oracle.com
CC:core-libs-dev@openjdk.java.net
On Nov 3 2011, at 17:40 , David Holmes wrote:
> Mike,
>
> I see that you have pushed a version of this change and that I was listed
as a reviewer. However I never saw an updated webrev and there was no response to
my query below. In fact I never saw any response to any of the reviewers comments
on this.
I missed your question about the range on an empty set. My comments below.
Removing EMPTY_SORTED_SET was the only other comment to my knowledge. Darryl
adapted the patch and EMPTY_SORTED_SET was not part of the commit. Since the
change was removal of a newly proposed field an additional review cycle wasn't
thought to be necessary. Perhaps incorrectly?
