com.livecode.bitwise: Refactor bit shifts.

peter-b · peter-b · commit cc09e546eed8 · 2015-03-05T13:46:27.000Z
Require people to specify "left" or "right", and force the shift count
to be positive.
diff --git a/libscript/src/bitwise.mlc b/libscript/src/bitwise.mlc
@@ -27,7 +27,7 @@ public foreign handler MCBitwiseEvalBitwiseOr(in Left as LCInt, in Right as LCIn
 public foreign handler MCBitwiseEvalBitwiseNot(in Operand as LCInt, out Value as LCInt) as undefined binds to "<builtin>"
 public foreign handler MCBitwiseEvalBitwiseXor(in Left as LCInt, in Right as LCInt, out Value as LCInt) as undefined binds to "<builtin>"
 
-public foreign handler MCBitwiseEvalBitwiseShift(in Target as LCInt, in IsRight as CBool, in Shift as LCInt, out Value as LCInt) as undefined binds to "<builtin>"
+public foreign handler MCBitwiseEvalBitwiseShift(in Target as LCInt, in IsRight as CBool, in Shift as LCUInt, out Value as LCInt) as undefined binds to "<builtin>"
 
 --
 
@@ -140,16 +140,16 @@ Returns: 	The result of bit-shifting <Operand> by <Shift> places.
 
 Example:
 	variable tVar
-	put 7 shifted by 2 bitwise into tVar -- tVar contains 28
+	put 7 shifted left by 2 bitwise into tVar -- tVar contains 28
 
 Description:
-Shifts the bits of <Operand> left or right. If neither direction is specified, then shifts left. Shifting the bits of <Operand> left by x is equivalent to multiplying by 2^x. Shifting by a negative value is equivalent to shifting by its absolute value in the opposite direction.
+Shifts the bits of <Operand> left or right. Shifting the bits of <Operand> left by x is equivalent to multiplying by 2^x.
 
 Tags: Bitwise operations
 */
 
 syntax BitwiseShift is postfix operator with precedence 1
-    <Operand: Expression> "shifted" ("left" <IsRight=false> | "right" <IsRight=true> | <IsRight=false>) "by" <Shift: Expression> "bitwise"
+    <Operand: Expression> "shifted" ("left" <IsRight=false> | "right" <IsRight=true>) "by" <Shift: Expression> "bitwise"
 begin
     MCBitwiseEvalBitwiseShift(Operand, IsRight, Shift, output)
 end syntax
diff --git a/libscript/src/module-bitwise.cpp b/libscript/src/module-bitwise.cpp
@@ -36,27 +36,32 @@ extern "C" MC_DLLEXPORT void MCBitwiseEvalBitwiseNot(integer_t p_operand, intege
     r_output = ~((int32_t)p_operand);
 }
 
-extern "C" MC_DLLEXPORT void MCBitwiseEvalBitwiseShift(integer_t p_operand, bool p_is_right, integer_t p_shift, integer_t& r_output)
+extern "C" MC_DLLEXPORT void MCBitwiseEvalBitwiseShift(integer_t p_operand, bool p_is_right, uinteger_t p_shift, integer_t& r_output)
 {
-	/* Ensure that the shift amount is always positive */
-	if (p_shift < 0)
-	{
-		/* Overflow check (why would anyone do this?) */
-		if (INTEGER_MIN == p_shift)
-			++p_shift;
-
-		p_shift = -p_shift;
-		p_is_right = !p_is_right;
-	}
-
-	/* Shifting integer_t by more than 31 bits is undefined */
-	integer_t t_max_shift = (sizeof(integer_t) << 3) - 1;
+	/* Maximum shift amount for which the C operator is defined */
+	uinteger_t t_max_shift = (sizeof(integer_t) << 3) - 1;
 	p_shift = MCMin (p_shift, t_max_shift);
 
+	integer_t t_shifted;
 	if (p_is_right)
-		r_output = p_operand >> p_shift;
+	{
+		/* It's okay to shift right by any amount */
+		t_shifted = p_operand >> p_shift;
+	}
 	else
-		r_output = p_operand << p_shift;
+	{
+		t_shifted = p_operand << p_shift;
+
+		/* Overflow check */
+		if (p_operand != t_shifted >> p_shift)
+		{
+			MCErrorCreateAndThrow (kMCGenericErrorTypeInfo, "reason",
+			                       MCSTR("overflow in bitwise operation"), nil);
+			return;
+		}
+	}
+
+	r_output = t_shifted;
 }
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
diff --git a/tests/lcb/stdlib/bitwise.lcb b/tests/lcb/stdlib/bitwise.lcb
@@ -17,6 +17,7 @@ along with LiveCode.  If not see <http://www.gnu.org/licenses/>.  */
 
 module com.livecode.bitwise.tests
 
+use com.livecode.__INTERNAL._testlib
 use com.livecode.bitwise
 
 public handler TestAnd()
@@ -35,8 +36,42 @@ public handler TestNot()
 	test "not" when bitwise not -5 is 4
 end handler
 
+handler TestShift_OverflowPositive()
+	return 5 shifted left by 29 bitwise
+end handler
+handler TestShift_OverflowNegative()
+	return -5 shifted left by 29 bitwise
+end handler
 public handler TestShift()
-	test "shift" when 7 shifted by 2 bitwise is 28
+	-- 0…00101 → 0…01010
+	test "shift left (+ve)" when 5 shifted left by 1 bitwise is 10
+	-- 1…11011 → 1…10110
+	test "shift left (-ve)" when -5 shifted left by 1 bitwise is -10
+	-- 0…00000 → 0…00000
+	test "shift left (0)" when 0 shifted left by 1 bitwise is 0
+
+	-- 0…00101 → 0…00010
+	test "shift right (+ve)" when 5 shifted right by 1 bitwise is 2
+	-- 1…11011 → 1…11101
+	test "shift right (-ve)" when -5 shifted right by 1 bitwise is -3
+	-- 0…00000 → 0…00000
+	test "shift right (0)" when 0 shifted right by 1 bitwise is 0
+
+	-- Shift right by any amount is permitted
+	test "shift right (+ve, big)" when 5 shifted right by 255 bitwise is 0
+	test "shift right (-ve, big)" when -5 shifted right by 255 bitwise is -1
+	test "shift right (0, big)" when 0 shifted right by 255 bitwise is 0
+
+	-- Shift left is only permitted if no overflow occurs
+	-- 0…00101 → 01010…0
+	test "shift left (+ve, limit)" when 5 shifted left by 28 bitwise is 1342177280
+	-- 1…11011 → 10110…0
+	test "shift left (-ve, limit)" when -5 shifted left by 28 bitwise is -1342177280
+	-- 1…11111 → 10000…0
+	test "shift left (-ve, edge)" when -1 shifted left by 31 bitwise is -2147483648
+
+	MCUnitTestHandlerThrows(TestShift_OverflowPositive, "shift left (+ve, overflow")
+	MCUnitTestHandlerThrows(TestShift_OverflowNegative, "shift left (-ve, overflow")
 end handler
 
 end module
diff --git a/toolchain/lc-compile/test.mlc b/toolchain/lc-compile/test.mlc
@@ -498,7 +498,7 @@ public handler testBitwise(inout xResults as List)
 	put bitwise not 5 into tVar
 	testLog("Bitwise", "BitwiseNot", tVar is - 6, xResults)
 
-	put 7 shifted by 2 bitwise into tVar 
+	put 7 shifted left by 2 bitwise into tVar 
 	testLog("Bitwise", "BitwiseShift", tVar is 28, xResults)
 	
 end handler
