Some cleanup concerning byte arrays.

5 files changed, 51 insertions, 79 deletions

diff --git a/ArrayUtils.cpp b/ArrayUtils.cpp
index 58031390..1a91b053 100644
--- a/ArrayUtils.cpp
+++ b/ArrayUtils.cpp
@@ -388,10 +388,7 @@ void ArrayUtils::convertLengthToSize(ArrayType const& _arrayType, bool _pad) con
 {
 	if (_arrayType.getLocation() == ArrayType::Location::Storage)
 	{
-		if (_arrayType.isByteArray())
-			m_context << u256(31) << eth::Instruction::ADD
-				<< u256(32) << eth::Instruction::SWAP1 << eth::Instruction::DIV;
-		else if (_arrayType.getBaseType()->getStorageSize() <= 1)
+		if (_arrayType.getBaseType()->getStorageSize() <= 1)
 		{
 			unsigned baseBytes = _arrayType.getBaseType()->getStorageBytes();
 			if (baseBytes == 0)
@@ -465,82 +462,61 @@ void ArrayUtils::accessIndex(ArrayType const& _arrayType) const
 
 	m_context << legalAccess;
 	// stack: <base_ref> <index>
-	if (_arrayType.isByteArray())
-		switch (location)
-		{
-		case ArrayType::Location::Storage:
-			// byte array index storage lvalue on stack (goal):
-			// <ref> <byte_number> = <base_ref + index / 32> <index % 32>
-			m_context << u256(32) << eth::Instruction::SWAP2;
+	m_context << eth::Instruction::SWAP1;
+	if (_arrayType.isDynamicallySized())
+	{
+		if (location == ArrayType::Location::Storage)
 			CompilerUtils(m_context).computeHashStatic();
-			// stack: 32 index data_ref
+		else if (location == ArrayType::Location::Memory)
+			m_context << u256(32) << eth::Instruction::ADD;
+	}
+	// stack: <index> <data_ref>
+	switch (location)
+	{
+	case ArrayType::Location::CallData:
+		if (!_arrayType.isByteArray())
+			m_context
+				<< eth::Instruction::SWAP1 << _arrayType.getBaseType()->getCalldataEncodedSize()
+				<< eth::Instruction::MUL;
+		m_context << eth::Instruction::ADD;
+		if (_arrayType.getBaseType()->isValueType())
+			CompilerUtils(m_context).loadFromMemoryDynamic(
+				*_arrayType.getBaseType(),
+				true,
+				!_arrayType.isByteArray(),
+				false
+			);
+		break;
+	case ArrayType::Location::Storage:
+		m_context << eth::Instruction::SWAP1;
+		if (_arrayType.getBaseType()->getStorageBytes() <= 16)
+		{
+			// stack: <data_ref> <index>
+			// goal:
+			// <ref> <byte_number> = <base_ref + index / itemsPerSlot> <(index % itemsPerSlot) * byteSize>
+			unsigned byteSize = _arrayType.getBaseType()->getStorageBytes();
+			solAssert(byteSize != 0, "");
+			unsigned itemsPerSlot = 32 / byteSize;
+			m_context << u256(itemsPerSlot) << eth::Instruction::SWAP2;
+			// stack: itemsPerSlot index data_ref
 			m_context
 				<< eth::Instruction::DUP3 << eth::Instruction::DUP3
 				<< eth::Instruction::DIV << eth::Instruction::ADD
-			// stack: 32 index (data_ref + index / 32)
+			// stack: itemsPerSlot index (data_ref + index / itemsPerSlot)
 				<< eth::Instruction::SWAP2 << eth::Instruction::SWAP1
 				<< eth::Instruction::MOD;
-			break;
-		case ArrayType::Location::CallData:
-			// no lvalue, just retrieve the value
-			m_context
-				<< eth::Instruction::ADD << eth::Instruction::CALLDATALOAD
-				<< ((u256(0xff) << (256 - 8)))  << eth::Instruction::AND;
-			break;
-		case ArrayType::Location::Memory:
-			solAssert(false, "Memory lvalues not yet implemented.");
-		}
-	else
-	{
-		// stack: <base_ref> <index>
-		m_context << eth::Instruction::SWAP1;
-		if (_arrayType.isDynamicallySized())
-		{
-			if (location == ArrayType::Location::Storage)
-				CompilerUtils(m_context).computeHashStatic();
-			else if (location == ArrayType::Location::Memory)
-				m_context << u256(32) << eth::Instruction::ADD;
+			if (byteSize != 1)
+				m_context << u256(byteSize) << eth::Instruction::MUL;
 		}
-		// stack: <index> <data_ref>
-		switch (location)
+		else
 		{
-		case ArrayType::Location::CallData:
-			m_context
-				<< eth::Instruction::SWAP1 << _arrayType.getBaseType()->getCalldataEncodedSize()
-				<< eth::Instruction::MUL << eth::Instruction::ADD;
-			if (_arrayType.getBaseType()->isValueType())
-				CompilerUtils(m_context).loadFromMemoryDynamic(*_arrayType.getBaseType(), true, true, false);
-			break;
-		case ArrayType::Location::Storage:
-			m_context << eth::Instruction::SWAP1;
-			if (_arrayType.getBaseType()->getStorageBytes() <= 16)
-			{
-				// stack: <data_ref> <index>
-				// goal:
-				// <ref> <byte_number> = <base_ref + index / itemsPerSlot> <(index % itemsPerSlot) * byteSize>
-				unsigned byteSize = _arrayType.getBaseType()->getStorageBytes();
-				solAssert(byteSize != 0, "");
-				unsigned itemsPerSlot = 32 / byteSize;
-				m_context << u256(itemsPerSlot) << eth::Instruction::SWAP2;
-				// stack: itemsPerSlot index data_ref
-				m_context
-					<< eth::Instruction::DUP3 << eth::Instruction::DUP3
-					<< eth::Instruction::DIV << eth::Instruction::ADD
-				// stack: itemsPerSlot index (data_ref + index / itemsPerSlot)
-					<< eth::Instruction::SWAP2 << eth::Instruction::SWAP1
-					<< eth::Instruction::MOD
-					<< u256(byteSize) << eth::Instruction::MUL;
-			}
-			else
-			{
-				if (_arrayType.getBaseType()->getStorageSize() != 1)
-					m_context << _arrayType.getBaseType()->getStorageSize() << eth::Instruction::MUL;
-				m_context << eth::Instruction::ADD << u256(0);
-			}
-			break;
-		case ArrayType::Location::Memory:
-			solAssert(false, "Memory lvalues not yet implemented.");
+			if (_arrayType.getBaseType()->getStorageSize() != 1)
+				m_context << _arrayType.getBaseType()->getStorageSize() << eth::Instruction::MUL;
+			m_context << eth::Instruction::ADD << u256(0);
 		}
+		break;
+	case ArrayType::Location::Memory:
+		solAssert(false, "Memory lvalues not yet implemented.");
 	}
 }
 
diff --git a/Compiler.cpp b/Compiler.cpp
index 8e263449..886565cb 100644
--- a/Compiler.cpp
+++ b/Compiler.cpp
@@ -254,7 +254,6 @@ void Compiler::appendCalldataUnpacker(TypePointers const& _typeParameters, bool
 
 void Compiler::appendReturnValuePacker(TypePointers const& _typeParameters)
 {
-	//@todo this can be also done more efficiently
 	unsigned dataOffset = 0;
 	unsigned stackDepth = 0;
 	for (TypePointer const& type: _typeParameters)
@@ -303,9 +302,6 @@ bool Compiler::visit(VariableDeclaration const& _variableDeclaration)
 bool Compiler::visit(FunctionDefinition const& _function)
 {
 	CompilerContext::LocationSetter locationSetter(m_context, _function);
-	//@todo to simplify this, the calling convention could by changed such that
-	// caller puts: [retarg0] ... [retargm] [return address] [arg0] ... [argn]
-	// although note that this reduces the size of the visible stack
 
 	m_context.startFunction(_function);
 
diff --git a/CompilerUtils.cpp b/CompilerUtils.cpp
index 45495114..8d3e9d2a 100644
--- a/CompilerUtils.cpp
+++ b/CompilerUtils.cpp
@@ -93,7 +93,7 @@ void CompilerUtils::storeInMemoryDynamic(Type const& _type, bool _padToWordBound
 		else
 		{
 			solAssert(type.getLocation() == ArrayType::Location::Storage, "Memory arrays not yet implemented.");
-			m_context << eth::Instruction::POP; //@todo
+			m_context << eth::Instruction::POP; // remove offset, arrays always start new slot
 			m_context << eth::Instruction::DUP1 << eth::Instruction::SLOAD;
 			// stack here: memory_offset storage_offset length_bytes
 			// jump to end if length is zero
diff --git a/LValue.cpp b/LValue.cpp
index 234072bc..02e6cbca 100644
--- a/LValue.cpp
+++ b/LValue.cpp
@@ -225,7 +225,8 @@ void StorageItem::setToZero(SourceLocation const&, bool _removeReference) const
 	else if (m_dataType.getCategory() == Type::Category::Struct)
 	{
 		// stack layout: storage_key storage_offset
-		// @todo this can be improved for packed types
+		// @todo this can be improved: use StorageItem for non-value types, and just store 0 in
+		// all slots that contain value types later.
 		auto const& structType = dynamic_cast<StructType const&>(m_dataType);
 		for (auto const& member: structType.getMembers())
 		{
@@ -245,7 +246,6 @@ void StorageItem::setToZero(SourceLocation const&, bool _removeReference) const
 	else
 	{
 		solAssert(m_dataType.isValueType(), "Clearing of unsupported type requested: " + m_dataType.toString());
-		// @todo actually use offset
 		if (!_removeReference)
 			CompilerUtils(m_context).copyToStackTop(sizeOnStack(), sizeOnStack());
 		if (m_dataType.getStorageBytes() == 32)
diff --git a/Types.h b/Types.h
index e1852bc7..e6d32d17 100644
--- a/Types.h
+++ b/Types.h
@@ -345,7 +345,7 @@ public:
 	explicit ArrayType(Location _location):
 		m_location(_location),
 		m_isByteArray(true),
-		m_baseType(std::make_shared<FixedBytesType>(8))
+		m_baseType(std::make_shared<FixedBytesType>(1))
 	{}
 	/// Constructor for a dynamically sized array type ("type[]")
 	ArrayType(Location _location, const TypePointer &_baseType):