[PATCH] D143882: [ADT] Add llvm::rotl and llvm::rotr to bit.h

[Kazu Hirata via Phabricator via llvm-commits]

kazu added inline comments.


================
Comment at: llvm/include/llvm/ADT/bit.h:366
+  if (R < 0)
+    return llvm::rotr(V, -R);
+
----------------
andrew.w.kaylor wrote:
> This is dead code. Because you defined N as unsigned, R will be converted to unsigned for the % operation on line 361 and the result will never be negative. It gets the right answer anyway, but it doesn't seem to be what you intended.
> 
> As an example, in your test "llvm::rotl<uint8_t>(0x53, -5)" this will happen
> 
> N = 8u; // (std::numeric_limits<uint8t>::digits)
> R = 0xFFFFFFFB % 8u; // (unsigned)R % N
> 
> So R = 3, and rotl(0x53, 3) yields 0x9A, as expected.
> 
> The point is, the code on lines 365-366 isn't doing anything. The same applies to lines 378-379 below.
> 
> This was reported by Coverity, BTW, which is why I happened to notice.
Thank you for catching and reporting this!  I'll post a follow-up patch to fix this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143882