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

[Thomas Stuefe]

On Fri, 30 Jul 2021 16:10:29 GMT, Thomas Stuefe <stuefe 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,
> 
>> 
>> 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.
> 
>> 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.

> @tstuefe I was not paying attention when C++ added overloading based on constness of return type so that's an interesting change, but also, does this get rid of the monstrosity:
> 
> ```
> return const_cast<Node const**>(
>     const_cast<ResourceHashtableBase*>(this)->lookup_node(hash, key));
> ```
> 
> ?

No, sorry, I think this monstrosity is the price one pays for const overloading a method but sharing the implementation. Plus doing all casts in the C++ correct very verbose fashion instead of the old boring C-style way.

That said, in the above code, I am not 100% sure the cast for the return value is even needed. Since we go from non-const pointer to const pointer, should that not work without a cast?

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

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