[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticASTKinds.td:223
+def note_constexpr_bit_cast_invalid_type : Note<
+  "cannot constexpr evaluate a bit_cast with a "
+  "%select{union|pointer|member pointer|volatile|struct with a reference member}0"
----------------
Quuxplusone wrote:
> rsmith wrote:
> > "constexpr evaluate" doesn't really mean anything. Also, the "struct with a reference member type X" case seems a bit strangely phrased (and it need not be a struct anyway...).
> > 
> > Maybe "cannot bit_cast {from|to}0 a {|type with a}1 {union|pointer|member pointer|volatile|reference}2 {type|member}1 in a constant expression"?
> Peanut gallery says: Surely wherever this message comes from should use the same wording as other similar messages: `"(this construction) is not allowed in a constant expression"`. That is, the diagnostics for
> ```
> constexpr int *foo(void *p) { return reinterpret_cast<int*>(p); }
> constexpr int *bar(void *p) { return std::bit_cast<int*>(p); }
> ```
> should be word-for-word identical except for the kind of cast they're complaining about.
> 
> (And if I had my pedantic druthers, every such message would say "...does not produce a constant expression" instead of "...is not allowed in a constant expression." But that's way out of scope for this patch.)
Sure, that seems more consistent. 


================
Comment at: clang/lib/AST/ExprConstant.cpp:5454-5457
+    case APValue::Indeterminate:
+      Info.FFDiag(BCE->getExprLoc(), diag::note_constexpr_access_uninit)
+          << 0 << 1;
+      return false;
----------------
rsmith wrote:
> I've checked the intent on the reflectors, and we should drop both this and the `APValue::None` check here. Bits corresponding to uninitialized or out-of-lifetime subobjects should just be left uninitialized (exactly like padding bits).
> 
> Example:
> 
> ```
> struct X { char c; int n; };
> struct Y { char data[sizeof(X)]; };
> constexpr bool test() {
>   X x = {1, 2};
>   Y y = __builtin_bit_cast(Y, x); // OK, y.data[1] - y.data[3] are APValue::Indeterminate
>   X z = __builtin_bit_cast(X, y); // OK, both members are initialized
>   return x.c == z.c && x.n == z.n;
> }
> static_assert(test());
> ```
You mean:
```
- struct Y { char data[sizeof(X)]; };
+ struct Y { unsigned char data[sizeof(X)]; };
```
...right? I added this test to the constexpr-builtin-bit-cast.cpp (as well of some of your other cases from the reflector).


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

https://reviews.llvm.org/D62825