[PATCH] D23035: [Support] Add LLVM_BITFIELD_WIDTH and LLVM_CHECKED_BITFIELD_ASSIGN macros.

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

You don't have any negative tests. Want to write some death tests behind NDEBUG?


================
Comment at: llvm/include/llvm/Support/Bitfields.h:29
@@ +28,3 @@
+    Type T;                                                                    \
+    ::memset(&T, 0xff, sizeof(T));                                             \
+    uint64_t Bits = T.Field;                                                   \
----------------
You're also relying on a bunch of other things here... Mostly that Type can be memset to all 1s without violating any of its invariants such as that this will create no trap representations.

I'm not sure how to word a static assert such that it catches this, but at the least you should check that it is standard layout and trivially copyable.

================
Comment at: llvm/include/llvm/Support/Bitfields.h:35-40
@@ +34,8 @@
+
+/// Does
+///
+///   DstContainer.DstField = Src;
+///
+/// asserts that Src fits inside Dst.  DstField must be a bitfield with
+/// an unsigned integral underlying type.
+///
----------------
I don't think this will read effectively aftor auto-brief. I think you need the first sentence to be a prose description.

================
Comment at: llvm/include/llvm/Support/Bitfields.h:45
@@ +44,3 @@
+/// \code
+///   Struct T {
+///     unsigned X : 2;
----------------
s/S/s/

================
Comment at: llvm/unittests/Support/BitfieldsTest.cpp:23-24
@@ +22,4 @@
+  };
+  // Can't stick LLVM_BITFIELD_WIDTH macro into EXPECT_EQ -- the two macros'
+  // magics are at odds.
+  int V;
----------------
That's odd, even with parentheses around the macro?


https://reviews.llvm.org/D23035