From 4622f31fa45f77baf9bb1149c094b5683c9a04dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Patrick=20L=C3=BChne?= <patrick-github@luehne.de>
Date: Tue, 27 Jun 2017 17:43:00 +0200
Subject: [PATCH] Made conditional effect variant more concise.

---
 lib/pddlparse/include/pddlparse/ASTForward.h  | 41 +++++++++----------
 .../src/pddlparse/detail/parsing/Effect.cpp   | 14 +++----
 .../tests/TestOfficialPDDLInstances.cpp       |  6 +--
 3 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/lib/pddlparse/include/pddlparse/ASTForward.h b/lib/pddlparse/include/pddlparse/ASTForward.h
index 8a83ce0..762a37f 100644
--- a/lib/pddlparse/include/pddlparse/ASTForward.h
+++ b/lib/pddlparse/include/pddlparse/ASTForward.h
@@ -150,6 +150,24 @@ class AtomicFormula : public detail::AtomicFormulaT
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 
+namespace detail
+{
+using LiteralT = Variant<
+	AtomicFormula,
+	NotPointer<AtomicFormula>>;
+}
+
+class Literal : public detail::LiteralT
+{
+	Literal() = delete;
+
+	using detail::LiteralT::LiteralT;
+};
+
+using Literals = std::vector<Literal>;
+
+////////////////////////////////////////////////////////////////////////////////////////////////////
+
 class Precondition;
 
 namespace detail
@@ -182,9 +200,8 @@ namespace detail
 {
 // TODO: add missing types
 using ConditionalEffectT = Variant<
-	AtomicFormula,
-	AndPointer<ConditionalEffect>,
-	NotPointer<ConditionalEffect>>;
+	Literal,
+	AndPointer<Literal>>;
 }
 
 class ConditionalEffect : public detail::ConditionalEffectT
@@ -234,24 +251,6 @@ class Type : public detail::TypeT
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 
-namespace detail
-{
-using LiteralT = Variant<
-	AtomicFormula,
-	NotPointer<AtomicFormula>>;
-}
-
-class Literal : public detail::LiteralT
-{
-	Literal() = delete;
-
-	using detail::LiteralT::LiteralT;
-};
-
-using Literals = std::vector<Literal>;
-
-////////////////////////////////////////////////////////////////////////////////////////////////////
-
 class Fact;
 
 namespace detail
diff --git a/lib/pddlparse/src/pddlparse/detail/parsing/Effect.cpp b/lib/pddlparse/src/pddlparse/detail/parsing/Effect.cpp
index 9dcaaee..7b927e1 100644
--- a/lib/pddlparse/src/pddlparse/detail/parsing/Effect.cpp
+++ b/lib/pddlparse/src/pddlparse/detail/parsing/Effect.cpp
@@ -20,7 +20,7 @@ namespace detail
 
 std::experimental::optional<ast::Effect> parseEffectBody(Context &context, ASTContext &astContext, VariableStack &variableStack);
 std::experimental::optional<ast::ConditionalEffect> parseConditionalEffect(Context &context, ASTContext &astContext, VariableStack &variableStack);
-std::experimental::optional<ast::ConditionalEffect> parseConditionalEffectBody(Context &context, ASTContext &astContext, VariableStack &variableStack);
+std::experimental::optional<ast::Literal> parseConditionalEffectBody(Context &context, ASTContext &astContext, VariableStack &variableStack);
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 
@@ -95,7 +95,7 @@ std::experimental::optional<ast::ConditionalEffect> parseConditionalEffect(Conte
 
 	std::experimental::optional<ast::ConditionalEffect> conditionalEffect;
 
-	if ((conditionalEffect = parseAnd<ast::ConditionalEffect>(context, astContext, variableStack, parseConditionalEffectBody)))
+	if ((conditionalEffect = parseAnd<ast::Literal>(context, astContext, variableStack, parseConditionalEffectBody)))
 		return std::move(conditionalEffect.value());
 
 	return parseConditionalEffectBody(context, astContext, variableStack);
@@ -103,7 +103,7 @@ std::experimental::optional<ast::ConditionalEffect> parseConditionalEffect(Conte
 
 ////////////////////////////////////////////////////////////////////////////////////////////////////
 
-std::experimental::optional<ast::ConditionalEffect> parseConditionalEffectBody(Context &context, ASTContext &astContext, VariableStack &variableStack)
+std::experimental::optional<ast::Literal> parseConditionalEffectBody(Context &context, ASTContext &astContext, VariableStack &variableStack)
 {
 	auto &tokenizer = context.tokenizer;
 
@@ -130,12 +130,12 @@ std::experimental::optional<ast::ConditionalEffect> parseConditionalEffectBody(C
 	tokenizer.seek(position);
 
 	// Now, test supported expressions
-	std::experimental::optional<ast::ConditionalEffect> conditionalEffect;
+	std::experimental::optional<ast::Literal> literal;
 
-	if ((conditionalEffect = parseNot<ast::ConditionalEffect>(context, astContext, variableStack, parseAtomicFormula))
-		|| (conditionalEffect = parseAtomicFormula(context, astContext, variableStack)))
+	if ((literal = parseNot<ast::AtomicFormula>(context, astContext, variableStack, parseAtomicFormula))
+		|| (literal = parseAtomicFormula(context, astContext, variableStack)))
 	{
-		return std::move(conditionalEffect.value());
+		return std::move(literal.value());
 	}
 
 	tokenizer.seek(expressionIdentifierPosition);
diff --git a/lib/pddlparse/tests/TestOfficialPDDLInstances.cpp b/lib/pddlparse/tests/TestOfficialPDDLInstances.cpp
index 9b85721..93f70c3 100644
--- a/lib/pddlparse/tests/TestOfficialPDDLInstances.cpp
+++ b/lib/pddlparse/tests/TestOfficialPDDLInstances.cpp
@@ -358,7 +358,7 @@ TEST_CASE("[PDDL instances] The official PDDL instances are parsed correctly", "
 		const auto &effectWhen4 = effectAnd->arguments[4].get<pddl::ast::WhenPointer<pddl::ast::Precondition, pddl::ast::ConditionalEffect>>();
 		// TODO: check name of declaration
 		CHECK(effectWhen4->argumentLeft.get<pddl::ast::NotPointer<pddl::ast::Precondition>>()->argument.get<pddl::ast::AtomicFormula>().is<pddl::ast::PredicatePointer>());
-		CHECK(effectWhen4->argumentRight.get<pddl::ast::AtomicFormula>().is<pddl::ast::PredicatePointer>());
+		CHECK(effectWhen4->argumentRight.get<pddl::ast::Literal>().get<pddl::ast::AtomicFormula>().is<pddl::ast::PredicatePointer>());
 		const auto &effectForAll5 = effectAnd->arguments[5].get<pddl::ast::ForAllPointer<pddl::ast::Effect>>();
 		REQUIRE(effectForAll5->parameters.size() == 1);
 		CHECK(effectForAll5->parameters[0]->name == "oldsurface");
@@ -366,7 +366,7 @@ TEST_CASE("[PDDL instances] The official PDDL instances are parsed correctly", "
 		const auto &effectForAll5When = effectForAll5->argument.get<pddl::ast::WhenPointer<pddl::ast::Precondition, pddl::ast::ConditionalEffect>>();
 		// TODO: check name of declaration
 		CHECK(effectForAll5When->argumentLeft.get<pddl::ast::AtomicFormula>().is<pddl::ast::PredicatePointer>());
-		CHECK(effectForAll5When->argumentRight.get<pddl::ast::NotPointer<pddl::ast::ConditionalEffect>>()->argument.get<pddl::ast::AtomicFormula>().is<pddl::ast::PredicatePointer>());
+		CHECK(effectForAll5When->argumentRight.get<pddl::ast::Literal>().get<pddl::ast::NotPointer<pddl::ast::AtomicFormula>>()->argument.is<pddl::ast::PredicatePointer>());
 		const auto &effectForAll6 = effectAnd->arguments[6].get<pddl::ast::ForAllPointer<pddl::ast::Effect>>();
 		REQUIRE(effectForAll6->parameters.size() == 1);
 		CHECK(effectForAll6->parameters[0]->name == "oldpaint");
@@ -378,6 +378,6 @@ TEST_CASE("[PDDL instances] The official PDDL instances are parsed correctly", "
 		const auto &effectForAll9When = effectForAll9->argument.get<pddl::ast::WhenPointer<pddl::ast::Precondition, pddl::ast::ConditionalEffect>>();
 		// TODO: check name of declaration
 		CHECK(effectForAll9When->argumentLeft.get<pddl::ast::AtomicFormula>().is<pddl::ast::PredicatePointer>());
-		CHECK(effectForAll9When->argumentRight.get<pddl::ast::NotPointer<pddl::ast::ConditionalEffect>>()->argument.get<pddl::ast::AtomicFormula>().is<pddl::ast::PredicatePointer>());
+		CHECK(effectForAll9When->argumentRight.get<pddl::ast::Literal>().get<pddl::ast::NotPointer<pddl::ast::AtomicFormula>>()->argument.is<pddl::ast::PredicatePointer>());
 	}
 }