Address feedback from code review.

dbulnes · Jan 26, 2017 · 5b7cc01 · 5b7cc01
1 parent 9bcbd93
commit 5b7cc01
Show file tree

Hide file tree

Showing 5 changed files with 12 additions and 16 deletions.
diff --git a/docs/control-structures.rst b/docs/control-structures.rst
@@ -395,7 +395,7 @@ Currently, Solidity automatically generates a runtime exception in the following
 #. If your contract receives Ether via a public function without ``payable`` modifier (including the constructor and the fallback function).
 #. If your contract receives Ether via a public accessor function.
 
-Internally, Solidity performs an "invalid jump" when an exception is thrown and thus causes the EVM to revert all changes made to the state. The reason for this is that there is no safe way to continue execution, because an expected effect did not occur. Because we want to retain the atomicity of transactions, the safest thing to do is to revert all changes and make the whole transaction (or at least call) without effect.
+Internally, Solidity performs an "invalid jump" when a user-provided exception is thrown. In contrast, it performs an invalid (i.e., non-existent) operation if a runtime exception is encountered. In both cases, this causes the EVM to revert all changes made to the state. The reason for this is that there is no safe way to continue execution, because an expected effect did not occur. Because we want to retain the atomicity of transactions, the safest thing to do is to revert all changes and make the whole transaction (or at least call) without effect.
 
 .. index:: ! assembly, ! asm, ! evmasm
 

diff --git a/libsolidity/codegen/CompilerContext.cpp b/libsolidity/codegen/CompilerContext.cpp
@@ -222,9 +222,9 @@ CompilerContext& CompilerContext::appendInvalid()
 
 CompilerContext& CompilerContext::appendConditionalInvalid()
 {
-	eth::AssemblyItem falseTag = appendConditionalJump();
-	eth::AssemblyItem endTag = appendJumpToNew();
-	return *this << falseTag << Instruction::INVALID << endTag;
+	*this << Instruction::ISZERO;
+	eth::AssemblyItem afterTag = appendConditionalJump();
+	return *this << Instruction::INVALID << afterTag;
 }
 
 void CompilerContext::resetVisitedNodes(ASTNode const* _node)

diff --git a/libsolidity/codegen/ContractCompiler.cpp b/libsolidity/codegen/ContractCompiler.cpp
@@ -918,9 +918,9 @@ eth::AssemblyPointer ContractCompiler::cloneRuntime()
 	a << Instruction::DELEGATECALL;
 	//Propagate error condition (if DELEGATECALL pushes 0 on stack).
 	a << Instruction::ISZERO;
-	eth::AssemblyItem falseTag = a.appendJumpI();
-	eth::AssemblyItem endTag = a.appendJump().tag();
-	a << falseTag << Instruction::INVALID << endTag;
+	a << Instruction::ISZERO;
+	eth::AssemblyItem afterTag = a.appendJumpI();
+	a << Instruction::INVALID << afterTag;
 	//@todo adjust for larger return values, make this dynamic.
 	a << u256(0x20) << u256(0) << Instruction::RETURN;
 	return make_shared<eth::Assembly>(a);

diff --git a/test/libsolidity/Assembly.cpp b/test/libsolidity/Assembly.cpp
@@ -117,7 +117,7 @@ BOOST_AUTO_TEST_CASE(location_test)
 	AssemblyItems items = compileContract(sourceCode);
 	vector<SourceLocation> locations =
 		vector<SourceLocation>(17, SourceLocation(2, 75, n)) +
-		vector<SourceLocation>(32, SourceLocation(20, 72, n)) +
+		vector<SourceLocation>(30, SourceLocation(20, 72, n)) +
 		vector<SourceLocation>{SourceLocation(42, 51, n), SourceLocation(65, 67, n)} +
 		vector<SourceLocation>(2, SourceLocation(58, 67, n)) +
 		vector<SourceLocation>(3, SourceLocation(20, 72, n));

diff --git a/test/libsolidity/SolidityExpressionCompiler.cpp b/test/libsolidity/SolidityExpressionCompiler.cpp
@@ -337,21 +337,17 @@ BOOST_AUTO_TEST_CASE(arithmetics)
 					   byte(Instruction::ADD),
 					   byte(Instruction::DUP2),
 					   byte(Instruction::ISZERO),
-					   byte(Instruction::PUSH1), 0x1e,
+					   byte(Instruction::ISZERO),
+					   byte(Instruction::PUSH1), 0x1d,
 					   byte(Instruction::JUMPI),
-					   byte(Instruction::PUSH1), 0x20,
-					   byte(Instruction::JUMP),
-					   byte(Instruction::JUMPDEST),
 					   byte(Instruction::INVALID),
 					   byte(Instruction::JUMPDEST),
 					   byte(Instruction::MOD),
 					   byte(Instruction::DUP2),
 					   byte(Instruction::ISZERO),
-					   byte(Instruction::PUSH1), 0x2a,
+					   byte(Instruction::ISZERO),
+					   byte(Instruction::PUSH1), 0x26,
 					   byte(Instruction::JUMPI),
-					   byte(Instruction::PUSH1), 0x2c,
-					   byte(Instruction::JUMP),
-					   byte(Instruction::JUMPDEST),
 					   byte(Instruction::INVALID),
 					   byte(Instruction::JUMPDEST),
 					   byte(Instruction::DIV),