Using @implSpec in AbstractMap, AbstractCollection, AbstractList, etc

Fri Jan 3 02:14:32 UTC 2014

OK, as you wish - this change is clear progress!

On Thu, Jan 2, 2014 at 3:48 PM, Chris Hegarty <chris.hegarty at oracle.com>wrote:

>
> On 2 Jan 2014, at 22:02, Martin Buchholz <martinrb at google.com> wrote:
>
> As the subject says, these changes should be applied to all of the
> so-called "skeletal implementations" in java.util.
>
>
> You may have noticed that I used a different subject line in the bug ;-)
>
> There is a *lot* of (previously unavoidable (painful memories)) javadoc
> duplication in java.util, some of which could now be undone.
>
>
> If you are agreeable, then I'd like to push just the AbstractMap changes
> as they are to JDK 9 and JDK 8. We can then immediately follow up with the
> additional skeletal types, under a different bug number for JDK 9, and
> evaluate the feasibility of putting it into JDK 8.
>
> This is just a matter of timing, it is getting very late in the JDK 8
> release cycle. There is a specific problem in CHM stemming from
> AbstractMap, which I would like to resolve without growing the scope of the
> changes and risk.
>
> -Chris.
>
>
>
> On Thu, Jan 2, 2014 at 8:03 AM, Chris Hegarty <chris.hegarty at oracle.com>wrote:
>
>> Hi Doug,
>>
>> I agree with your changes to AbstractMap. I’ve taken your patch, removed
>> the superfluous paragraph tags, and generated a webrev.
>>    http://cr.openjdk.java.net/~chegar/AbstractMap_implSpec/webrev/
>>
>> I also ran specdiff to see what else may be impacted by this. It shows
>> that the only spec changes are to AbstractMap itself, and to
>> ConcurrentHashMap size and isEmpty. What we want.
>>
>> http://cr.openjdk.java.net/~chegar/AbstractMap_implSpec/specdiff/overview-summary.html
>>
>> I know it is late in the JDK 8 game, but I see this as a serious bug in
>> the documentation, and it needs to be addressed.
>>
>> Conservatively, one could argue that minimally pasting the appropriate
>> specs into CHM size and isEmpty would be sufficient to resolve the problem,
>> but I think your first proposal is a more complete solution. Given the
>> above specdiff, I would be confident to request this change for inclusion
>> in JDK 8.
>>
>> I filed the following bug to track this issue:
>>    https://bugs.openjdk.java.net/browse/JDK-8031133
>>
>> -Chris.
>>
>> On 28 Dec 2013, at 13:47, Doug Lea <dl at cs.oswego.edu> wrote:
>>
>> >
>> > There might have been some mis-communication about whether the
>> > new @implSpec tag would be used in existing java.util.AbstractX
>> > classes. I had assumed that it would be, and pulled out a few
>> > javadoc overrides in java.util.concurrent classes that would
>> > not be needed if @implSpec were used. With @implSpec, you do
>> > not need to paste in the interface-level spec in an overridden
>> > method just to suppress inclusion of (incorrect) doc comments
>> > about the default implementation.
>> >
>> > Is it too late to do this for JDK8? It is easy -- just add a line
>> > with @implSpec for each defined method. AbstractMap diffs
>> > below. (They could be tweaked to get rid of now-unnecessary
>> > paragraph markup, but I don't think this would improve display.)
>> >
>> > If this isn't done, then minimally we'd need to paste in
>> > interface-level specs in ConcurrentHashMap.{size, isEmpty}
>> > since the JDK8 javadocs as they stand are wrong. There
>> > may be other cases as well.
>> >
>> > ...
>> >
>> >
>> > diff -r 8bc258c021a3 src/share/classes/java/util/AbstractMap.java
>> > --- a/src/share/classes/java/util/AbstractMap.java    Thu Nov 21
>> 09:23:03 2013 -0800
>> > +++ b/src/share/classes/java/util/AbstractMap.java    Sat Dec 28
>> 08:33:42 2013 -0500
>> > @@ -78,6 +78,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation returns <tt>entrySet().size()</tt>.
>> >      */
>> >     public int size() {
>> > @@ -87,6 +88,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation returns <tt>size() == 0</tt>.
>> >      */
>> >     public boolean isEmpty() {
>> > @@ -96,6 +98,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation iterates over <tt>entrySet()</tt>
>> searching
>> >      * for an entry with the specified value.  If such an entry is
>> found,
>> >      * <tt>true</tt> is returned.  If the iteration terminates without
>> > @@ -126,6 +129,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation iterates over <tt>entrySet()</tt>
>> searching
>> >      * for an entry with the specified key.  If such an entry is found,
>> >      * <tt>true</tt> is returned.  If the iteration terminates without
>> > @@ -157,6 +161,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation iterates over <tt>entrySet()</tt>
>> searching
>> >      * for an entry with the specified key.  If such an entry is found,
>> >      * the entry's value is returned.  If the iteration terminates
>> without
>> > @@ -191,6 +196,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation always throws an
>> >      * <tt>UnsupportedOperationException</tt>.
>> >      *
>> > @@ -206,6 +212,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation iterates over <tt>entrySet()</tt>
>> searching for an
>> >      * entry with the specified key.  If such an entry is found, its
>> value is
>> >      * obtained with its <tt>getValue</tt> operation, the entry is
>> removed
>> > @@ -255,6 +262,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation iterates over the specified map's
>> >      * <tt>entrySet()</tt> collection, and calls this map's <tt>put</tt>
>> >      * operation once for each entry returned by the iteration.
>> > @@ -276,6 +284,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation calls <tt>entrySet().clear()</tt>.
>> >      *
>> >      * <p>Note that this implementation throws an
>> > @@ -302,6 +311,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation returns a set that subclasses {@link
>> AbstractSet}.
>> >      * The subclass's iterator method returns a "wrapper object" over
>> this
>> >      * map's <tt>entrySet()</tt> iterator.  The <tt>size</tt> method
>> > @@ -358,6 +368,7 @@
>> >     /**
>> >      * {@inheritDoc}
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation returns a collection that subclasses
>> {@link
>> >      * AbstractCollection}.  The subclass's iterator method returns a
>> >      * "wrapper object" over this map's <tt>entrySet()</tt> iterator.
>> > @@ -425,6 +436,7 @@
>> >      * <tt>equals</tt> method works properly across different
>> implementations
>> >      * of the <tt>Map</tt> interface.
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation first checks if the specified object is
>> this map;
>> >      * if so it returns <tt>true</tt>.  Then, it checks if the specified
>> >      * object is a map whose size is identical to the size of this map;
>> if
>> > @@ -478,6 +490,7 @@
>> >      * <tt>m1</tt> and <tt>m2</tt>, as required by the general contract
>> of
>> >      * {@link Object#hashCode}.
>> >      *
>> > +     * @implSpec
>> >      * <p>This implementation iterates over <tt>entrySet()</tt>, calling
>> >      * {@link Map.Entry#hashCode hashCode()} on each element (entry) in
>> the
>> >      * set, and adding up the results.
>>
>>
>