From 1264290d574858c0438dd72dfc91b17d0b2049e8 Mon Sep 17 00:00:00 2001
From: Christian Parpart <christian@parpart.family>
Date: Fri, 26 Oct 2018 13:38:53 +0200
Subject: [PATCH] [Yul] ExpressionJoiner: code cleanup

* ensure public API is only containing `run(Block&)`, all the rest is private API / implementation details
* adding some comments to class data members to quicker understand their meaning
* eliminate unnecessary `operator()(If&)` as it's not changing default behaviour of `ASTModifier`
* simplify readability of `visit(Expression&)`'s impl, also moving assert's into "isLatestStatementVarDeclOf", as this one is already ensuring exactly that.
* ctor impl's use of ReferenceCounter use shortened.
* renamed and improved `isLatestStatementVarDeclOf` to better match its meaning (especially since it's only used once)
---
 libyul/optimiser/ExpressionJoiner.cpp | 30 +++++++--------------------
 libyul/optimiser/ExpressionJoiner.h   | 26 +++++++++++------------
 2 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/libyul/optimiser/ExpressionJoiner.cpp b/libyul/optimiser/ExpressionJoiner.cpp
index c39574979..7e57a629e 100644
--- a/libyul/optimiser/ExpressionJoiner.cpp
+++ b/libyul/optimiser/ExpressionJoiner.cpp
@@ -46,20 +46,6 @@ void ExpressionJoiner::operator()(FunctionCall& _funCall)
 	handleArguments(_funCall.arguments);
 }
 
-void ExpressionJoiner::operator()(If& _if)
-{
-	visit(*_if.condition);
-	(*this)(_if.body);
-}
-
-void ExpressionJoiner::operator()(Switch& _switch)
-{
-	visit(*_switch.expression);
-	for (auto& _case: _switch.cases)
-		// Do not visit the case expression, nothing to join there.
-		(*this)(_case.body);
-}
-
 void ExpressionJoiner::operator()(Block& _block)
 {
 	resetLatestStatementPointer();
@@ -79,13 +65,11 @@ void ExpressionJoiner::visit(Expression& _e)
 	if (_e.type() == typeid(Identifier))
 	{
 		Identifier const& identifier = boost::get<Identifier>(_e);
-		if (isLatestStatementVarDeclOf(identifier) && m_references[identifier.name] == 1)
+		if (isLatestStatementVarDeclJoinable(identifier))
 		{
 			VariableDeclaration& varDecl = boost::get<VariableDeclaration>(*latestStatement());
-			assertThrow(varDecl.variables.size() == 1, OptimizerException, "");
-			assertThrow(varDecl.value, OptimizerException, "");
-
 			_e = std::move(*varDecl.value);
+
 			// Delete the variable declaration (also get the moved-from structure back into a sane state)
 			*latestStatement() = Block();
 
@@ -103,9 +87,7 @@ void ExpressionJoiner::run(Block& _ast)
 
 ExpressionJoiner::ExpressionJoiner(Block& _ast)
 {
-	ReferencesCounter counter;
-	counter(_ast);
-	m_references = counter.references();
+	m_references = ReferencesCounter::countReferences(_ast);
 }
 
 void ExpressionJoiner::handleArguments(vector<Expression>& _arguments)
@@ -154,7 +136,7 @@ Statement* ExpressionJoiner::latestStatement()
 		return &m_currentBlock->statements.at(m_latestStatementInBlock);
 }
 
-bool ExpressionJoiner::isLatestStatementVarDeclOf(Identifier const& _identifier)
+bool ExpressionJoiner::isLatestStatementVarDeclJoinable(Identifier const& _identifier)
 {
 	Statement const* statement = latestStatement();
 	if (!statement || statement->type() != typeid(VariableDeclaration))
@@ -162,5 +144,7 @@ bool ExpressionJoiner::isLatestStatementVarDeclOf(Identifier const& _identifier)
 	VariableDeclaration const& varDecl = boost::get<VariableDeclaration>(*statement);
 	if (varDecl.variables.size() != 1 || !varDecl.value)
 		return false;
-	return varDecl.variables.at(0).name == _identifier.name;
+	assertThrow(varDecl.variables.size() == 1, OptimizerException, "");
+	assertThrow(varDecl.value, OptimizerException, "");
+	return varDecl.variables.at(0).name == _identifier.name && m_references[_identifier.name] == 1;
 }
diff --git a/libyul/optimiser/ExpressionJoiner.h b/libyul/optimiser/ExpressionJoiner.h
index df18e58f1..4f06cc0ff 100644
--- a/libyul/optimiser/ExpressionJoiner.h
+++ b/libyul/optimiser/ExpressionJoiner.h
@@ -73,29 +73,29 @@ class NameCollector;
 class ExpressionJoiner: public ASTModifier
 {
 public:
-	virtual void operator()(FunctionalInstruction&) override;
-	virtual void operator()(FunctionCall&) override;
-	virtual void operator()(If&) override;
-	virtual void operator()(Switch&) override;
-	virtual void operator()(Block& _block) override;
-
-	using ASTModifier::visit;
-	virtual void visit(Expression& _e) override;
-
 	static void run(Block& _ast);
+
 private:
 	explicit ExpressionJoiner(Block& _ast);
 
+	void operator()(Block& _block) override;
+	void operator()(FunctionalInstruction&) override;
+	void operator()(FunctionCall&) override;
+
+	using ASTModifier::visit;
+	void visit(Expression& _e) override;
+
 	void handleArguments(std::vector<Expression>& _arguments);
 
 	void decrementLatestStatementPointer();
 	void resetLatestStatementPointer();
 	Statement* latestStatement();
-	bool isLatestStatementVarDeclOf(Identifier const& _identifier);
+	bool isLatestStatementVarDeclJoinable(Identifier const& _identifier);
 
-	Block* m_currentBlock = nullptr;
-	size_t m_latestStatementInBlock = 0;
-	std::map<std::string, size_t> m_references;
+private:
+	Block* m_currentBlock = nullptr;            ///< Pointer to currently block holding the visiting statement.
+	size_t m_latestStatementInBlock = 0;        ///< Offset to m_currentBlock's statements of the last visited statement.
+	std::map<std::string, size_t> m_references; ///< Holds reference counts to all variable declarations in current block.
 };
 
 }