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`.

I think this is not an improvement, sorry. Now, a number of methods became non-const, including `iterate`, and that non-constness propagates. `iterate` itself does not change the table, and it can be reasonably called from const contexts. There are actually a number of callers which arguably should be const but aren't (e.g. `ClassLoaderStatsClosure::print()`.

I think a better approach would be to have both const and non-const versions, e.g. for `bucket_at()`:


  Node** bucket_at(unsigned index) ...
  const Node* const * bucket_at(unsigned index) const ...


This way, the compiler chooses the const version in const code, giving you the benefit of a read-only V*, and in non-const code you get access to the innards of V*. No awkward const casts needed.

Above code makes Coleen's new unlink() from https://github.com/openjdk/jdk/pull/4938 compile in non-const form.

Note how `ResourceHashTable::lookup_node()` already follows this pattern. You deleted the non-const version, but it made sense.

Same with `::get`:


const V* get(K const& key) const;
V* get(K const& key);


Making the Key references const is fine though.

..Thomas

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

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