+1 Map/ConcurrentMap.putIfAbsent/remove/replace: missing a close '}' in the doc code snippets.
That's a good test. Needs updating to include the bug id in the meta-data. Paul. On Oct 18, 2013, at 9:34 PM, Mike Duigou <mike.dui...@oracle.com> wrote: > > On Oct 17 2013, at 07:36 , Paul Sandoz <paul.san...@oracle.com> wrote: > >> >> On Oct 17, 2013, at 1:52 AM, Mike Duigou <mike.dui...@oracle.com> wrote: >> >>> >>> On Oct 16 2013, at 05:34 , Paul Sandoz <paul.san...@oracle.com> wrote: >>> >>>> On Oct 16, 2013, at 1:52 PM, David Holmes <david.hol...@oracle.com> wrote: >>>>>>> Perhaps HashMap's implementations should throw CME? >>>>>>> >>>>>> >>>>>> Perhaps, seems to be going beyond the call of duty. My inclination is >>>>>> not to bother. It becomes most relevant with forEach since the consumer >>>>>> will have side-effects that might make it easier to unintentionally slip >>>>>> in a modification to the map itself. >>>>> >>>>> I think there is a lot to be said for consistency. >>>> >>>> Yes, i was proposing to consistently not support it for non-traversal >>>> methods :-) >>> >>> >>> I have prepared an updated webrev removing all of the non-traversal CME >>> throwing. >>> >>> http://cr.openjdk.java.net/~mduigou/JDK-8024688/2/webrev/ >>> >> >> Code looks good. >> >> Agree with David there is still some work to do cleaning up the doc. >> >> e.g.: >> >> On concurrent map this: >> >> * @implNote This implementation assumes that the ConcurrentMap cannot >> * contain null values and {@code get()} returning null unambiguously means >> * the key is absent. Implementations which support null values >> * <strong>must</strong> override this default implementation. >> >> and this variant of the same thing: >> >> * The default implementation assumes that the ConcurrentMap cannot >> * contain null values and get() returning null unambiguously means the key >> * is absent. Implementations which support null values >> * <strong>must</strong> override this default implementation. >> >> should be impl specs not notes. (It could also be pulled up into the class >> rather than repeating for each default method.) May get reformulated if you >> state "The default implementation is equivalent to...". > > Done. > >> >> For: >> >> /** >> * {@inheritDoc} >> * >> * @implNote The default implementation may retry these steps when multiple >> * threads attempt updates. >> * >> * The default implementation assumes that the ConcurrentMap cannot >> * contain null values and get() returning null unambiguously means the key >> * is absent. Implementations which support null values >> * <strong>must</strong> override this default implementation. >> * >> * @throws UnsupportedOperationException {@inheritDoc} >> * @throws ClassCastException {@inheritDoc} >> * @throws NullPointerException {@inheritDoc} >> * @since 1.8 >> */ >> @Override >> default V computeIfAbsent(K key, >> Function<? super K, ? extends V> mappingFunction) { >> Objects.requireNonNull(mappingFunction); >> V v, newValue; >> return ((v = get(key)) == null && >> (newValue = mappingFunction.apply(key)) != null && >> (v = putIfAbsent(key, newValue)) == null) ? newValue : v; >> } >> >> there is no retry. > > Fixed > >>> It does bother me to be throwing out "good information" by not throwing the >>> CMEs but I'm willing to go with the flow. As a practical matter later >>> reintroduction of even valid error detection would almost certainly be >>> difficult. (https://bugs.openjdk.java.net/browse/JDK-5045147 for one >>> example). >>> >> >> I still think we can solve that with a statement on Map (my preference is to >> be DRY). An impl supporting CME should override the defaults, which is not >> that different conceptually to what is said about synchronization/atomicity >> on Map or null values on ConcurrentMap. > > I haven't tried to address this. My inclination is to instead document the > behaviour or the implementing classes. I don't like the notes in Map > referring to what ConcurrentMap impls must do. > > Mike
