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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith 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"
----------------
"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"?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9732
+def err_bit_cast_type_size_mismatch : Error<
+  "__builtin_bit_cast source size does not equal destination size (%0 and %1)">;
 } // end of sema component.
----------------
We would usually use `vs` rather than `and` here.


================
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;
----------------
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());
```


================
Comment at: clang/lib/AST/ExprConstant.cpp:5614-5615
+    SmallVector<uint8_t, 8> Bytes;
+    if (!Buffer.readObject(Offset, SizeOf, Bytes))
+      return APValue::IndeterminateValue();
+
----------------
This should fail with a diagnostic if `T` is not a byte-like type (an unsigned narrow character type or `std::byte`), due to [basic.indet]p2.


================
Comment at: clang/lib/CodeGen/CGExprComplex.cpp:472-475
+    Address Temp = CGF.CreateTempAlloca(CGF.ConvertType(Op->getType()), Align,
+                                        "bit_cast.temp");
+    CGF.EmitAnyExprToMem(Op, Temp, Op->getType().getQualifiers(),
+                         /*isInit=*/false);
----------------
This seems to unnecessarily force an extra round-trip through memory. `Op` is an lvalue, so it already describes a value in memory, and you should be able to use `EmitLValue(Op)` to get the lvalue you want to load from.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:2045-2048
+    Address Temp = CGF.CreateTempAlloca(CGF.ConvertType(E->getType()), Align,
+                                        "bit_cast.temp");
+    CGF.EmitAnyExprToMem(E, Temp, E->getType().getQualifiers(),
+                         /*isInit=*/false);
----------------
Likewise here.


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

https://reviews.llvm.org/D62825