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.
2. The comparator method is using raw types.
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.
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.
5. What if I want to create an empty set sorted set with supplied comparator?
6. Why not implement an EmptyNavigableSet instead since the bug was entered
before 1.6?
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?
