[PATCH] D30336: [APInt] Add APInt::extractBits() method to extract APInt subrange

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: lib/Support/APInt.cpp:624
+  assert(bitPosition < BitWidth && (numBits + bitPosition) <= BitWidth &&
+    "Illegal bit extraction");
+
----------------
This is very nit-picky, but I find `numBits > 0` much more natural than `0 < numBits`.
And I'd suggest splitting up the second assert into two, to make it clearer what went wrong if it fires.



================
Comment at: lib/Support/APInt.cpp:628
+  if (isSingleWord())
+    return APInt(numBits, VAL >> loBit);
+
----------------
For the single-word case we know bitPosition is in the first word, so there's no need for the remainder operation in `whichBit`. I'd suggest moving the if-statement before computing loBit, and just using bitPosition in the shift.


================
Comment at: lib/Support/APInt.cpp:645
+  uint64_t *pDst = Result.pVal;
+  for (unsigned word = loWord; word < hiWord; ++word, ++pDst) {
+    uint64_t w0 = pVal[word + 0];
----------------
I'm not sure this loop is correct. It will never set `Result.pVal[hiWord]`?

The only test case I see that hits this loop is `EXPECT_EQ(-8388481, i256.extractBits(128, 1).getSExtValue());`

In that one, the result is computed from three words, but indeed fits into two words. 

But what if it didn't -- what about `i256.extractBits(129, 1)`?


================
Comment at: unittests/ADT/APIntTest.cpp:1441
+
+  APInt i256(256, -16776961 /* 0xFFFFFFFFFFFFFFFFFFFFFFFFFF0000FF */, true);
+  EXPECT_EQ(255, i256.extractBits(16, 0));
----------------
Would it be possible to use hex constants here and in the expectations somehow? Especially for the last one, it's hard to see what the expected result should be..


Repository:
  rL LLVM

https://reviews.llvm.org/D30336