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

[Thomas Stuefe]

On Fri, 30 Jul 2021 04:16:59 GMT, Ioi Lam <iklam at openjdk.org> wrote:

> `ResourceHashtableBase::iterate()` is declared `const`, but it can actually change the contents of the table. The same is true for `ResourceHashtableBase::get()`, which returns a non-`const` pointer to the value, allowing the caller to modify it.
> 
> We should declare these two functions as non-`const`. This will also remove a lot of ugly `const_cast<>` code.
> 
> The `iterate()` API is tightened such that the `do_entry()` function can modify the `value` but not the `key`.

Hi Ioi,

In my opinion iterate() is not non-const since it in itself does not modify anything. It calls a user provided hook that may do. 

But that's getting philosophical, and separating an non-const iterate from a const const_iterate is fine by me, if the contract is enforcable.

Cheers, Thomas

src/hotspot/share/utilities/resourceHash.hpp line 81:

> 79:   Node const** lookup_node(unsigned hash, K const& key) const {
> 80:     return const_cast<Node const**>(
> 81:         const_cast<ResourceHashtableBase*>(this)->lookup_node(hash, key));

I would keep this the const version of lookup_node(), since that seems more correct. I would use it in contains() instead of using get(), which does too much.

src/hotspot/share/utilities/resourceHash.hpp line 115:

> 113: 
> 114:   bool contains(K const& key) const {
> 115:     return const_cast<ResourceHashtableBase*>(this)->get(key) != NULL;

Wonder if we can macro out the pattern of un-constifying "this". I would hope that C++ develops and gives us a better looking solution.

src/hotspot/share/utilities/resourceHash.hpp line 129:

> 127: 
> 128:   const V* get(K const& key) const {
> 129:     return const_cast<const V*>(const_cast<ResourceHashtableBase*>(this)->get(key));

I don't think the return cast is needed.

src/hotspot/share/utilities/resourceHash.hpp line 212:

> 210:       NODE* node = *bucket;
> 211:       while (node != NULL) {
> 212:         bool cont = iter->do_entry(const_cast<K const&>(node->_key), const_cast<VALUE>(node->_value));

I don't grok the casts, why are they needed? Node is already const type here, no?

-------------

Changes requested by stuefe (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4942