This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Generalize the conditions for testing bitcasts between long double, double and int128
ClosedPublic

Authored by mstorsjo on Oct 12 2021, 1:04 PM.

Details

Summary

MSVC targets also have a 64 bit long double, as do MinGW targets on ARM.
This hasn't been noticed in CI because the MSVC configurations there run
with _LIBCPP_HAS_NO_INT128 defined.

This avoids assuming that either __int128_t or double is equal in size to
long double. i386 MinGW targets have sizeof(long double) == 10, which
doesn't match any of the tested types.

Unfortunately, this relies on using nonstandard SIZEOF_* defines.

Diff Detail

Event Timeline

mstorsjo created this revision.Oct 12 2021, 1:04 PM
mstorsjo requested review of this revision.Oct 12 2021, 1:04 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 12 2021, 1:04 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante accepted this revision as: Mordante.Oct 13 2021, 10:16 AM
Mordante added a subscriber: Mordante.

It seems we already relay on __SIZEOF_* in our code, including in <bitset> so I don't see that as a big issue.
LGTM modulo one minor remark.

libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
233

In __config there's this definition

#ifndef __SIZEOF_INT128__
#define _LIBCPP_HAS_NO_INT128
#endif

So this test can be removed.

Quuxplusone accepted this revision.Oct 13 2021, 10:28 AM
Quuxplusone added inline comments.
libcxx/test/std/numerics/bit/bit.cast/bit_cast.pass.cpp
233

LGTM mod this comment as well. Also, I don't think you should be "silently hiding" if __SIZEOF_DOUBLE__ is not defined. In the unlikely event that it's not defined, I think we want to be told about it via -Wundefined-macro-is-zero or whatever that warning's name is.

So when this is addressed, you'll just have

#if __SIZEOF_LONG_DOUBLE__ == __SIZEOF_DOUBLE__
        test_roundtrip_through<double, false>(i);
#endif
#if defined(__SIZEOF_INT128__) && __SIZEOF_LONG_DOUBLE__ == __SIZEOF_INT128__
        test_roundtrip_through<__int128_t, false>(i);
        test_roundtrip_through<__uint128_t, false>(i);
#endif
This revision is now accepted and ready to land.Oct 13 2021, 10:28 AM
mstorsjo updated this revision to Diff 379475.Oct 13 2021, 11:22 AM

Simplify the ifdefs, as suggested