RFR: 8255389: ConcurrentHashTable::NoOp omits return in non-void return method

[Kim Barrett]

On Mon, 26 Oct 2020 09:41:12 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

> Static analysis complains there is a non-void return method without a return statement:
> 
>   struct NoOp {
>     void operator()(VALUE*) {}
>     const VALUE& operator()() {} // <--- here
>     void operator()(bool, VALUE*) {}
>   } noOp;
> 
> AFAICS, this is UB, and we have seen cases like these break compilers in other places. Not in this case, though, because `noOp` is only used as the default functor in `remove`, which does not use this getter-like definition. Still, it would be good to remove that risky definition, so that it is not used accidentally.

Changes requested by kbarrett (Reviewer).

src/hotspot/share/utilities/concurrentHashTable.hpp line 197:

> 195:   // Note it only accepts the VALUE, and does not define methods with
> 196:   // non-void VALUE returns. Doing so would require defining the neutral
> 197:   // value for VALUE.

I don't think this comment is needed.  What's needed is a description of the DELETE_FN in the declaration of remove(), but that kind of template parameter requirements documentation is entirely missing from ConcurrentHashTable (and most other templates in HotSpot).

src/hotspot/share/utilities/concurrentHashTable.hpp line 200:

> 198:   struct NoOp {
> 199:     void operator()(VALUE*) {}
> 200:     void operator()(bool, VALUE*) {}

The two argument call operator also appears to be unused.

src/hotspot/share/utilities/concurrentHashTable.hpp line 198:

> 196:   // non-void VALUE returns. Doing so would require defining the neutral
> 197:   // value for VALUE.
> 198:   struct NoOp {

A better name for this class would be something like IgnoreValue.

src/hotspot/share/utilities/concurrentHashTable.hpp line 201:

> 199:     void operator()(VALUE*) {}
> 200:     void operator()(bool, VALUE*) {}
> 201:   } noOp;

I don't think the noOp object is useful.  I think it would be clearer to just use a constructed object of this type in the one place it's used, i.e. `NoOp()` or (if name is changed) `IgnoreValue()`.

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

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