PATCH: using mixed types in MIN2/MAX2 functions

[Jesper Wilhelmsson]

Kim Barrett skrev 30/7/14 18:19:
> 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.

I think you have a really good point here. Is there a reason for the different 
types being used or should we simply converge to using the same type all over?
/Jesper


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