RFR: JDK-8299688: Adopt C++14 compatible std::bit_cast introduced in C++20 [v4]

[Justin King]

On Mon, 23 Jan 2023 07:42:40 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> Justin King has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Remove addressof as nobody should be overloading operator&
>>   
>>   Signed-off-by: Justin King <jcking at google.com>
>
> src/hotspot/share/utilities/bitCast.hpp line 46:
> 
>> 44: 
>> 45: // From and To are integrals of the same size. We can simply static_cast without changing the bit
>> 46: // representation.
> 
> The PrimitiveConversions version has a longer comment providing more justification for this implementation.
> Please retain that.

Restored the comment.

> src/hotspot/share/utilities/bitCast.hpp line 63:
> 
>> 61: template <typename To, typename From,
>> 62:           ENABLE_IF(sizeof(To) == sizeof(From) &&
>> 63:                     !std::is_same<From, To>::value &&
> 
> No need for this, since an integral type and an enum type are by definition different.  Similarly for the
> overload for the other direction.

Done.

> src/hotspot/share/utilities/bitCast.hpp line 191:
> 
>> 189:   return __builtin_bit_cast(To, from);
>> 190: #else
>> 191:   // Most modern compilers will produce optimal code for memcpy.
> 
> This comment is at odds with the comment in the preceding integer<->float conversion.  I think the
> comment there is correct and this one is not.

Believed I fixed this.

> src/hotspot/share/utilities/bitCast.hpp line 192:
> 
>> 190: #else
>> 191:   // Most modern compilers will produce optimal code for memcpy.
>> 192:   To to;
> 
> This default constructs To, which is potentially problematic, and is also an extra requirement.  I do see
> the corresponding restriction in the enabling constraints, but that isn't part of the Standard function's
> contract.  The problem can be avoided, but since I'm not sure we should be making this change at all...

Removed the requirement, since I am strictly only supported what PrimitiveConversions::cast supported for now.

> src/hotspot/share/utilities/globalDefinitions.hpp line 28:
> 
>> 26: #define SHARE_UTILITIES_GLOBALDEFINITIONS_HPP
>> 27: 
>> 28: #include "utilities/bitCast.hpp"
> 
> I think including bitCast here is problematic.  globalDefinitions is included nearly everywhere, while
> bitcasting is only needed in a few.  There is also a high risk of circular include dependencies when
> adding anything to this file's include list.  It looks like this addition happens to work today, but...
> And as noted elsewhere, I'm not sure some of the changes being made here to use bit_cast should
> be made anyway.  And if they should, perhaps the relevant code ought to be split out of this file.

Reverted changes to globalDefinitions.hpp.

> src/hotspot/share/utilities/globalDefinitions.hpp line 669:
> 
>> 667: inline jlong   jlong_cast   (jdouble x)  { return bit_cast<jlong>(x);  }
>> 668: inline julong  julong_cast  (jdouble x)  { return bit_cast<julong>(x); }
>> 669: inline jdouble jdouble_cast (jlong   x)  { return bit_cast<jdouble>(x);  }
> 
> I think the changes to the jXXX_cast's should not be made.  Instead, I think these should be eliminated
> and the relatively few uses should be changed to use PrimitiveConversions::cast / bit_cast directly.
> These functions have potentially unintended and problematic implicit conversions of the arguments.
> Such a change should have it's own PR.  There's already a bug related to this:
> https://bugs.openjdk.org/browse/JDK-8297539

Reverted changes to globalDefinitions.hpp.

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

PR: https://git.openjdk.org/jdk/pull/11865