Improves assembly and adds more tests.

5 files changed, 139 insertions, 35 deletions

diff --git a/libsolidity/ast/Types.cpp b/libsolidity/ast/Types.cpp
index 1c6003cd..8376b4bc 100644
--- a/libsolidity/ast/Types.cpp
+++ b/libsolidity/ast/Types.cpp
@@ -1702,7 +1702,7 @@ MemberList::MemberMap ArrayType::nativeMembers(ContractDefinition const*) const
 				TypePointers{},
 				strings{string()},
 				strings{string()},
-				isByteArray() ? FunctionType::Kind::ByteArrayPop : FunctionType::Kind::ArrayPop
+				FunctionType::Kind::ArrayPop
 			)});
 		}
 	}
diff --git a/libsolidity/ast/Types.h b/libsolidity/ast/Types.h
index 29c7db86..b2f34dee 100644
--- a/libsolidity/ast/Types.h
+++ b/libsolidity/ast/Types.h
@@ -916,7 +916,6 @@ public:
 		ArrayPush, ///< .push() to a dynamically sized array in storage
 		ArrayPop, ///< .pop() from a dynamically sized array in storage
 		ByteArrayPush, ///< .push() to a dynamically sized byte array in storage
-		ByteArrayPop, ///< .pop() from a dynamically sized byte array in storage
 		ObjectCreation, ///< array creation using new
 		Assert, ///< assert()
 		Require, ///< require()
diff --git a/libsolidity/codegen/ArrayUtils.cpp b/libsolidity/codegen/ArrayUtils.cpp
index 096b2d4e..d78e64a9 100644
--- a/libsolidity/codegen/ArrayUtils.cpp
+++ b/libsolidity/codegen/ArrayUtils.cpp
@@ -837,29 +837,23 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
 			switch and(slot_value, 1)
 			case 0 {
 				// short byte array
-				let length := and(div(slot_value, 2), 0x3f)
+				let length := and(div(slot_value, 2), 0x1f)
 				if iszero(length) { invalid() }
 
-				// Zero-out the suffix of the byte array by masking it.
-				// Do not zero-out the least significant byte, but mask the
-				// higher bits of the length.
-				// (((1<<(8 * (32 - length))) - 1) << 8) + 128
-				let mask := add(mul(0x100, sub(exp(0x100, sub(32, length)), 1)), 0x80)
-				slot_value := and(not(mask), slot_value)
-
-				// Reduce the length by 1
-				slot_value := sub(slot_value, 2)
+				// Zero-out the suffix inlcluding the least significant byte.
+				let mask := sub(exp(0x100, sub(33, length)), 1)
+				length := sub(length, 1)
+				slot_value := or(and(not(mask), slot_value), mul(length, 2))
 				sstore(ref, slot_value)
 			}
 			case 1 {
 				// long byte array
 				let length := div(slot_value, 2)
+				let slot := keccak256(0, 0x20)
 				mstore(0, ref)
-
 				switch length
 				case 32
 				{
-					let slot := keccak256(0, 0x20)
 					let data := sload(slot)
 					sstore(slot, 0)
 					data := and(data, not(0xff))
@@ -867,14 +861,14 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
 				}
 				default
 				{
-					let slot := div(sub(length, 1), 32)
-					let offset := and(sub(length, 1), 0x1f)
-					slot := add(keccak256(0, 0x20), slot)
+					let slot_offset := div(sub(length, 1), 32)
+					let length_offset := and(sub(length, 1), 0x1f)
+					slot := add(slot, slot_offset)
 					let data := sload(slot)
 
 					// Zero-out the suffix of the byte array by masking it.
 					// ((1<<(8 * (32 - offset))) - 1)
-					let mask := sub(exp(0x100, sub(32, offset)), 1)
+					let mask := sub(exp(0x100, sub(32, length_offset)), 1)
 					data := and(not(mask), data)
 					sstore(slot, data)
 
@@ -887,8 +881,7 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
 		m_context << Instruction::POP;
 	}
 	else
-	{		
-
+	{
 		// stack: ArrayReference
 		retrieveLength(_type);
 		// stack: ArrayReference oldLength
@@ -897,7 +890,6 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
 		m_context << Instruction::ISZERO;
 		m_context.appendConditionalInvalid();
 
-
 		// Stack: ArrayReference oldLength
 		m_context << u256(1) << Instruction::SWAP1 << Instruction::SUB;
 		// Stack ArrayReference newLength
@@ -905,7 +897,7 @@ void ArrayUtils::popStorageArrayElement(ArrayType const& _type) const
 		// Stack ArrayReference newLength ArrayReference newLength;
 		accessIndex(_type, false);
 		// Stack: ArrayReference newLength storage_slot byte_offset
-		StorageItem(m_context, _type).setToZero(SourceLocation(), true);
+		StorageItem(m_context, *_type.baseType()).setToZero(SourceLocation(), true);
 		// Stack: ArrayReference newLength
 		m_context << Instruction::SWAP1 << Instruction::SSTORE;
 	}
diff --git a/libsolidity/codegen/ExpressionCompiler.cpp b/libsolidity/codegen/ExpressionCompiler.cpp
index ac7610fc..93d440c8 100644
--- a/libsolidity/codegen/ExpressionCompiler.cpp
+++ b/libsolidity/codegen/ExpressionCompiler.cpp
@@ -866,7 +866,6 @@ bool ExpressionCompiler::visit(FunctionCall const& _functionCall)
 				StorageByteArrayElement(m_context).storeValue(*type, _functionCall.location(), true);
 			break;
 		}
-		case FunctionType::Kind::ByteArrayPop:
 		case FunctionType::Kind::ArrayPop:
 		{
 			_functionCall.expression().accept(*this);
@@ -1359,22 +1358,13 @@ bool ExpressionCompiler::visit(MemberAccess const& _memberAccess)
 					break;
 				}
 		}
-		else if (member == "push")
+		else if (member == "push" || member == "pop")
 		{
 			solAssert(
 				type.isDynamicallySized() &&
 				type.location() == DataLocation::Storage &&
 				type.category() == Type::Category::Array,
-				"Tried to use .push() on a non-dynamically sized array"
-			);
-		}
-		else if (member == "pop")
-		{
-			solAssert(
-				type.isDynamicallySized() &&
-				type.location() == DataLocation::Storage &&
-				type.category() == Type::Category::Array,
-				"Tried to use .pop() on a non-dynamically sized array"
+				"Tried to use ." + member + "() on a non-dynamically sized array"
 			);
 		}
 		else
diff --git a/test/libsolidity/SolidityEndToEndTest.cpp b/test/libsolidity/SolidityEndToEndTest.cpp
index 41101223..b0ab15c8 100644
--- a/test/libsolidity/SolidityEndToEndTest.cpp
+++ b/test/libsolidity/SolidityEndToEndTest.cpp
@@ -5115,7 +5115,6 @@ BOOST_AUTO_TEST_CASE(array_pop)
 {
 	char const* sourceCode = R"(
 		contract c {
-			uint256 a;
 			uint[] data;
 			function test() public returns (uint x, uint l) {
 				data.push(7);
@@ -5131,6 +5130,86 @@ BOOST_AUTO_TEST_CASE(array_pop)
 	ABI_CHECK(callContractFunction("test()"), encodeArgs(1, 0));
 }
 
+BOOST_AUTO_TEST_CASE(array_pop_uint16_transition)
+{
+	char const* sourceCode = R"(
+		contract c {
+			uint16[] data;
+			function test() public returns (uint16 x, uint16 y, uint16 z) {
+				for (uint i = 1; i <= 48; i++)
+					data.push(uint16(i));
+				for (uint j = 1; j <= 10; j++)
+					data.pop();
+				x = data[data.length - 1];
+				for (uint k = 1; k <= 10; k++)
+					data.pop();
+				y = data[data.length - 1];
+				for (uint l = 1; l <= 10; l++)
+					data.pop();
+				z = data[data.length - 1];
+				for (uint m = 1; m <= 18; m++)
+					data.pop();
+			}
+		}
+	)";
+	compileAndRun(sourceCode);
+	ABI_CHECK(callContractFunction("test()"), encodeArgs(38, 28, 18));
+	BOOST_CHECK(storageEmpty(m_contractAddress));
+}
+
+BOOST_AUTO_TEST_CASE(array_pop_uint24_transition)
+{
+	char const* sourceCode = R"(
+		contract c {
+			uint24[] data;
+			function test() public returns (uint24 x, uint24 y) {
+				for (uint i = 1; i <= 30; i++)
+					data.push(uint24(i));
+				for (uint j = 1; j <= 10; j++)
+					data.pop();
+				x = data[data.length - 1];
+				for (uint k = 1; k <= 10; k++)
+					data.pop();
+				y = data[data.length - 1];
+				for (uint l = 1; l <= 10; l++)
+					data.pop();
+			}
+		}
+	)";
+	compileAndRun(sourceCode);
+	ABI_CHECK(callContractFunction("test()"), encodeArgs(20, 10));
+	BOOST_CHECK(storageEmpty(m_contractAddress));
+}
+
+BOOST_AUTO_TEST_CASE(array_pop_array_transition)
+{
+	char const* sourceCode = R"(
+		contract c {
+			uint16[] inner = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16];
+			uint16[][] data;
+			function test() public returns (uint x, uint y, uint z) {
+				for (uint i = 1; i <= 48; i++)
+					data.push(inner);
+				for (uint j = 1; j <= 10; j++)
+					data.pop();
+				x = data[data.length - 1][0];
+				for (uint k = 1; k <= 10; k++)
+					data.pop();
+				y = data[data.length - 1][1];
+				for (uint l = 1; l <= 10; l++)
+					data.pop();
+				z = data[data.length - 1][2];
+				for (uint m = 1; m <= 18; m++)
+					data.pop();
+				delete inner;
+			}
+		}
+	)";
+	compileAndRun(sourceCode);
+	ABI_CHECK(callContractFunction("test()"), encodeArgs(1, 2, 3));
+	BOOST_CHECK(storageEmpty(m_contractAddress));
+}
+
 BOOST_AUTO_TEST_CASE(array_pop_empty_exception)
 {
 	char const* sourceCode = R"(
@@ -5252,6 +5331,50 @@ BOOST_AUTO_TEST_CASE(byte_array_pop_storage_empty_long)
 	BOOST_CHECK(storageEmpty(m_contractAddress));
 }
 
+BOOST_AUTO_TEST_CASE(byte_array_pop_masking_long)
+{
+	char const* sourceCode = R"(
+		contract c {
+			bytes data;
+			function test() public returns (bytes) {
+				for (uint i = 0; i < 34; i++)
+					data.push(3);
+				data.pop();
+				return data;
+			}
+		}
+	)";
+	compileAndRun(sourceCode);
+	ABI_CHECK(callContractFunction("test()"), encodeArgs(
+		u256(0x20),
+		u256(33),
+		asString(fromHex("0303030303030303030303030303030303030303030303030303030303030303")),
+		asString(fromHex("03"))
+	));
+}
+
+BOOST_AUTO_TEST_CASE(byte_array_pop_copy_long)
+{
+	char const* sourceCode = R"(
+		contract c {
+			bytes data;
+			function test() public returns (bytes) {
+				for (uint i = 0; i < 33; i++)
+					data.push(3);
+				for (uint j = 0; j < 4; j++)
+					data.pop();
+				return data;
+			}
+		}
+	)";
+	compileAndRun(sourceCode);
+	ABI_CHECK(callContractFunction("test()"), encodeArgs(
+		u256(0x20),
+		u256(29),
+		asString(fromHex("0303030303030303030303030303030303030303030303030303030303"))
+	));
+}
+
 BOOST_AUTO_TEST_CASE(external_array_args)
 {
 	char const* sourceCode = R"(