[PATCH] D17207: [ADT] Fix PointerEmbeddedInt when the underlying type is uintptr_t.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.

================
Comment at: include/llvm/ADT/PointerEmbeddedInt.h:48-50
@@ -44,1 +47,5 @@
 
+  enum RawValue_t {
+    RawValue
+  };
+
----------------
Rather than this, use an empty struct? That's a better tag type as it is literally not passed in the ABI, just changes the overload resolution at compile time.

The _t suffix is also not terribly common in LLVM. I'd go with 'RawValueTag' personally.

================
Comment at: include/llvm/ADT/PointerEmbeddedInt.h:64-65
@@ -56,2 +63,4 @@
   PointerEmbeddedInt &operator=(IntT I) {
-    assert((I & Mask) == 0 && "Integer has bits outside those preserved!");
+    assert((std::is_signed<IntT>::value ? llvm::isInt<Bits>(I)
+                                        : llvm::isUInt<Bits>(I)) && 
+           "Integer has bits outside those preserved!");
----------------
These isInt and isUInt predicates are... really confusingly named. "Of course its an integer! Oh, it's asserting how many bits are in the integer..."

Anyways, if you feel like it, maybe a follow-up patch to name these predicates better? Definitely not relevant for this patch.

================
Comment at: unittests/ADT/PointerEmbeddedIntTest.cpp:53-54
@@ +52,4 @@
+
+  {
+    PointerEmbeddedInt<uintptr_t, CHAR_BIT> I = 42, J = 255;
+    EXPECT_EQ(42U, I);
----------------
Rather than all of the scopes, just use different names for the different types? Makes it much easier to read IMO.


Repository:
  rL LLVM

http://reviews.llvm.org/D17207