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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits