[PATCH] D154951: [clang][Interp] __builtin_bit_cast, Take 2

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/AST/Interp/Boolean.h:113
+  static Boolean bitcastFromMemory(const std::byte *Buff) {
+    bool Val = static_cast<bool>(*Buff);
+    return Boolean(Val);
----------------
tbaeder wrote:
> MitalAshok wrote:
> > Does this handle padding bits correctly? E.g., `__builtin_bit_cast(bool, (unsigned char) 0b10u)` should be false
> No, it doesn't. But clang has a completely different opinion about that test.
I think Clang diagnoses that as an invalid constant expression because of https://eel.is/c++draft/utilities#bit.cast-2.sentence-5 which we're allowed to do because of http://eel.is/c++draft/expr.const#5.31


================
Comment at: clang/lib/AST/Interp/Interp.h:1546
+  const Pointer &FromPtr = S.Stk.pop<Pointer>();
+  Pointer &ToPtr = S.Stk.peek<Pointer>();
+
----------------
Should we be checking that we don't have a member pointer at this point?


================
Comment at: clang/lib/AST/Interp/InterpBitcast.cpp:19
+
+// TODO: Try to e-duplicate the primitive and composite versions.
+
----------------
Alternatively: `e-duplicate2000`


================
Comment at: clang/lib/AST/Interp/InterpBitcast.cpp:71
+/// All offsets are in bytes.
+struct ByteTracker {
+  std::vector<bool> Initialized;
----------------
Don't we need to track this at the *bit* level instead of the *byte* level? e.g., padding bits in structures, anonymous bit-fields, `bool`, `_BitInt`, etc?


================
Comment at: clang/lib/AST/Interp/InterpBitcast.cpp:72
+struct ByteTracker {
+  std::vector<bool> Initialized;
+  std::vector<std::byte> Data;
----------------
`llvm::BitVector` instead of `std::vector<bool>`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154951