Coming in late but this looks like a very good change to me as well. Mike
On Jan 2 2014, at 23:06 , Chris Hegarty <chris.hega...@oracle.com> wrote: > On 3 Jan 2014, at 02:14, Martin Buchholz <marti...@google.com> wrote: > >> OK, as you wish - this change is clear progress! > > Thanks Martin. > > I pushed the AbstractMap changes to JDK 8 and JDK 9. > > 8031142 now tracks the changes to AbstractCollection and AbstractList > https://bugs.openjdk.java.net/browse/JDK-8031142 > > -Chris. > >> >> On Thu, Jan 2, 2014 at 3:48 PM, Chris Hegarty <chris.hega...@oracle.com> >> wrote: >> >> On 2 Jan 2014, at 22:02, Martin Buchholz <marti...@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.hega...@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 <d...@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. >>> >>> >> >
