[PATCH] D76323: [AST] Fix handling of long double and bool in __builtin_bit_cast

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:6365
+    const llvm::fltSemantics &Semantics = Info.Ctx.getFloatTypeSemantics(Ty);
+    unsigned NumBits = APFloat::semanticsSizeInBits(Semantics);
+    assert(NumBits % 8 == 0);
----------------
ldionne wrote:
> `semanticsSizeInBits` is the number of bits actually used in the type?
Yeah


================
Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:226-250
 typedef decltype(nullptr) nullptr_t;
 
 #ifdef __CHAR_UNSIGNED__
 // expected-note at +5 {{indeterminate value can only initialize an object of type 'unsigned char', 'char', or 'std::byte'; 'unsigned long' is invalid}}
 #else
 // expected-note at +3 {{indeterminate value can only initialize an object of type 'unsigned char' or 'std::byte'; 'unsigned long' is invalid}}
 #endif
----------------
>>! In D76323#1933791, @jfb wrote:
> Maybe you should test `nullptr` as well, given that it's all padding?

There are some existing `nullptr` padding tests here ^^^, which seem to cover this.


================
Comment at: clang/test/SemaCXX/constexpr-builtin-bit-cast.cpp:407
+
+constexpr bool test_bad_bool = bit_cast<bool>('A'); // expected-error {{must be initialized by a constant expression}} expected-note{{in call}}
+
----------------
jfb wrote:
> Sanity-check: `0` and `1` work? IIRC we had a discussion about whether `false` was `0` and `true` was `1`, and we concluded that `false` was indeed `0` but `true` wasn't specified to be `1`.
Yeah, they work, I added a test in the update. I don't think it really matters that `true` could technically be `2` in the standard here, since the representation on our implementation is always `1`.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76323/new/

https://reviews.llvm.org/D76323