PATCH: using mixed types in MIN2/MAX2 functions

[Kim Barrett]

On Jul 28, 2014, at 5:56 PM, Jesper Wilhelmsson <jesper.wilhelmsson at oracle.com> wrote:
> 
> Trying to get this discussion going again.
> 
> There isn't too much of non-trivial template usage in HotSpot today and I'm not sure I think it's worth complicating the code to avoid a few type casts.
> 
> How do other people feel about the non-trivial template usage?

For what it’s worth, the code I sent out earlier can be further
simplified.  It also contains a bug, because I didn't think through
the problem quite carefully enough.  I know how to fix the bug (and
also how to improve the tests so it would have been caught!).  If
there's still interest I can produce an update.

I have a very strong dislike for casts in most contexts.  The number
of casts and other unchecked conversions I'm running across in the
hotspot code base makes me cringe.

My understanding is that the proposed casts are to work around
semantically different types being used (e.g. command flags of uintx
type that actually represent size_t values), which happen to be
implemented using different primitive types on some small set of
platforms.  If that's true, a better solution would be to actually fix
the semantic type mismatches, though that may be more work.

Casts that are only needed for a small set of platforms (and which
might not even be included in Oracle testing) seem quite problematic
to me - casts generally make for more difficult to understand code,
and in a situation like this a future reader is going to have a hard
time understanding which casts are truly meaningful and which are
workarounds for unusual platforms.  Maintenance is also going to be
difficult: when is a cast needed in new code, and when can a cast in
existing code be eliminated?

"Non-trivial" is of course dependent on the reader.  And a few
comments might help the casual reader.

My biggest worry, from a portability standpoint, is the #include of
<limits>, which could run afoul of issues similar to
https://bugs.openjdk.java.net/browse/JDK-8007770, or to bugs in
<limits> itself - Boost.Config has several defect macros to describe
bugs encountered in various (generally quite old) versions of
<limits>.

That said, I don't have any attachment to that code.  There was a
question about whether mixed type support could be done in this
situation where different platforms are using different primitive
types.  It can.  Whether we actually want to do so is a different
question.

I think a simplified approach that just handles the specific known
problematic cases could also be done.  I think the amount of template
infrastructure involved in that might be less than the more general
approach taken in the code I sent.  That's kind of ugly, though (IMO)
still an improvement on cast littering.

A completely different approach to the problem would be to
conditionally add the needed extra overloads, using preprocessor
conditionalization based on the toolchain.  My impression is that the
coding conventions of this code base (attempt to) eschew such
conditionalizations in otherwise generic code, instead isolating such
stuff to platform/target/toolchain-specific files.  That might be a
little awkward to do in this case, but that might be seen by some as
more palatable than the template approach.  I don't like this any
better than the afore-mentioned template support for specific
additional types.