RFR: 8271525: ResourceHashtableBase::iterate() should not declared as const

[David Holmes]

On 31/07/2021 4:57 am, Ioi Lam wrote:
> On Fri, 30 Jul 2021 16:10:29 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:
> 
>> Hi Ioi,
>>
>>> Hi Thomas,
>>> I've added `const V* get(K const& key) const;` as you suggested.
>>> However, I don't think `iterate()` should be const, because we have code that actually modifies the table's contents:
>>
>> I disagree; iterate does not modify the table. Calling it from a const context (eg. to print the table) should be possible.
> 
> It does. The code I pointed below modifies the `counter` which is stored inside the table. It's possible to write a iterator that prints out all the counters, and resets all of them to zero. To me, this is a modification of the table. The current API does not forbid that, so it should not be declared `const`.
> 
> We need to be consistent:
> 
> - `V* get(key)` returns a `V*` that can be used to modify the contents in the table.
> - `iterate(iter)` passes a `V*` that can be used to modify the contents in the table.
> 
> So we should declare both of these functions const (or declare both of them non-const).
> 
> I think non-const makes more sense. I have a hard time understand why add/removing items are considered non-const, but modifying items are considered const.

I'm getting quite confused. We are talking about the const-ness of the 
_table_ right? Not the const-ness of the keys or values in the table. If 
you add/remove/update a mapping/entry that modifies the table and is 
non-const.

If you iterate and access an item what you do with the item is 
completely independent of the table. And the API for the table shouldn't 
be trying to dictate whether or not you can modify something currently 
in the table IMO.

David
----

>>> https://github.com/openjdk/jdk/blob/77fbd99f792c42bb92a240d38f35e3af25500f99/src/hotspot/share/logging/logAsyncWriter.cpp#L98-L107
>>>
>>> I'll try to refactor the code to have a `const_iterate()` which calls `ITER::do_entry(const&K, const &V)`.
>>
>> If ITER were not a template parameter but a real functor, this would make sense since we could say:
>>
>> ```
>> void iterate(iterator& it);
>> void iterate(const_iterator& it) const;
>> ```
>>
>> but with ITER being a template parameter, I don't know of a way to enforce the use of only const ITERs with a const_iterate. Therefore I am not sure a const_iterate would give us that much.
> 
> My current implementation of `const_iterate()` already enforces that `ITER::do_entry()` must be declared with `(K const&, V const&)`, due to this call:
> 
> 
> bool cont = iter->do_entry(const_cast<K const&>(node->_key), const_cast<VALUE>(node->_value));
> 
> 
> That's because `const_iterate()` instantiates `VALUE` as `V const&`.
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/4942
>