RFR : 8016446 : (m) Add override forEach/replaceAll to HashMap, Hashtable, IdentityHashMap, WeakHashMap, TreeMap

[Remi Forax]

On 06/14/2013 09:45 PM, Mike Duigou wrote:
> On Jun 14 2013, at 05:41 , Remi Forax wrote:
>
>> On 06/14/2013 12:55 PM, Paul Sandoz wrote:
>>> On Jun 14, 2013, at 12:12 PM, Remi Forax <forax at univ-mlv.fr> wrote:
>>>>> The following does not throw CME:
>>>>>
>>>>>              List<Integer> l = new ArrayList<>(Arrays.asList(1, 2));
>>>>>              for (Integer i : l) {
>>>>>                  l.remove(1);  // 2 is never encountered
>>>>>              }
>>>>>
>>>>> Where as the following does:
>>>>>
>>>>>              List<Integer> l = new ArrayList<>(Arrays.asList(1, 2, 3));
>>>>>              for (Integer i : l) {
>>>>>                  l.remove(1);
>>>>>              }
>>>>>
>>>>> Because the hasNext implementation does not check for modification. It's sad this also occurs for the default implementation of Iterable.forEach :-(
>>>>>
>>>>> This behaviour sucks.
>>>> devil advocate: why exactly, the iteration is finished when you remove the element ?
>>> The latter because a CME is thrown; the former because hasNext returns false.
>>>
>>> The above is an example of how a bug can be hidden depending on the state (# elements) of the collection.
>>>
>>>
>>>>> It would be a shame for overriding forEach/forEachRemaining implementations to conform to such behaviour when they can implement stronger/consistent failure guarantees.
>>>> While I could agree with you in theory, in practice I have seen several times codes that rely on this behaviour,
>>>> usually there is a bunch of method calls between the for loop and the list.remove() so this is not something that can be easily fixed.
>>> A bug none the less, yes?
>> In the codes I was referring to, there was always a way to know that the remove was done at the end by example by knowing that the last element was a special sentinel or by using a counter.
>> So is the following program bugged ?
>>
>> List<Integer> l = new ArrayList<>(Arrays.asList(1, 2, null));
>> for (Integer i : l) {
>>   if (i == null) {
>>     l.set(l.size() - 1, 3);  // change the last value to 3
>>   }
>> }
> I would consider it bad form at minimum and probably buggy. Why insist on walking right next to the minefield? Sometimes minefields are poorly mapped (spec ambiguity), mines are misplaced (bugs) or you might stray (your bugs).
>
> What is the objection to the much safer and general:
>
> List<Integer> l = new ArrayList<>(Arrays.asList(1, 2, null));
> for(ListIterator<Integer> each = l.listIterator(); each.hasNext();) {
>    Integer i = each;
>    if(i == null) {
>       each.set(3);
>    }
> }
>
> Just the lack of for(:) loop?

I don't think so, I think the issue is usually because the real code is 
more something like:

List<Integer> l = new ArrayList<>(Arrays.asList(1, 2, null));
for (Integer i : l) {
   blob.foo(l, i);
}

and in another module somewhere in the application

void bar(List<Integer> l, Integer i) {
   if (i == null) {
    l.set(l.size() - 1, 3);  // change the last value to 3
  }

and it appear that sometimes blob.foo calls several layers of methods 
that end up to call bar.

So while obviously your code is how to fix the problem, usually people 
don't even notice the problem
because the code of the loop and the code that does the mutation are far 
away from each other.

>
> Mike

Rémi