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
