Jason,
see inline and updated webrev:
http://cr.openjdk.java.net/~dmocek/7129185/webrev.01
Darryl
On Tue 24 Jan 2012 09:54:52 AM PST, Jason Mehrens wrote:
Darryl,
Here is my list of bugs:
1. The comparator method is using raw types. Should be Comparator<?
super E>.
The webrev didn't pick up on this change. The method is this:
public Comparator<? super E> comparator() {
return null;
}
2. The readResolve method comment is just wrong and the method
implementation is redundant of the default behavior.
readResolve has been removed.
3. What if I want to create an empty set navigable set with supplied
comparator? Extending is not an option.
This is the one issue I wanted to discuss...is this necessary? I was
thinking about how this would be implemented. You would need to supply
a comparator to the emptyNavigableSet. Other empty* methods don't take
parameters and adding a method to supply a comparator would require an
additional method.
Darryl
Here are my lists of RFEs:
5. Why not extend EmptySet so you don't have to reimplement the
optimized methods?
6. Why not create a default access static final reference named
EMPTY_NAVIGABLE_SET inside of the EmptyNavigableSet and use that in
readResolve and in Collections.emptyNavigableSet? That gives you the
nice singleton behavior and on demand class loading.
7. CCE lacks a descriptive message that you normally get if you used
Class.cast or just an implicit cast.
Jason
> Date: Mon, 23 Jan 2012 15:19:01 -0800
> From: darryl.mo...@oracle.com
> To: core-libs-dev@openjdk.java.net
> Subject: Code Review Request Bug #7129185:(coll) Please add
Collections.emptyNavigableSet()
>
> Re-sending this with the synopsis in the subject line (and the correct
> bug #).
>
> Hello core-libs. Please review this patch to fix Bug #7129185. This
> fix addresses comments made by Jason Mehrens to the commit of the fix
> for bug #4533691, including adding a Collections.emptyNavigableSet
> method. Tests are included.
>
> Webrev, can be found here:
> http://cr.openjdk.java.net/~dmocek/7129185/webrev.00
>
> Thanks,
> Darryl
