From a3d0a942e4977b81d93051463190b5a887141616 Mon Sep 17 00:00:00 2001
From: Robert Bendun <rob.bendun@gmail.com>
Date: Thu, 18 Aug 2022 21:32:19 +0200
Subject: [PATCH] Ensure when adding new operators we add them right

Operators not only are needed to be defined by their implementation in
Interpreter constructor, but also are needed to be included in
precedense resolution.

To prevent partial definition we predefine number of builtin operators
as numeric constant and test against it in all relevant places.
---
 src/interpreter.cc | 88 +++++++++++++++++++++++++++-------------------
 src/musique.hh     |  3 +-
 src/parser.cc      |  6 ++++
 3 files changed, 59 insertions(+), 38 deletions(-)

diff --git a/src/interpreter.cc b/src/interpreter.cc
index 8223fa3..e7b9cb0 100644
--- a/src/interpreter.cc
+++ b/src/interpreter.cc
@@ -711,52 +711,66 @@ error:
 			global.force_define("program_change", pgmchange);
 		}
 
+		// Operators definition table
+		static constexpr auto Operators = std::array {
+			std::tuple { "+", plus_minus_operator<std::plus<>> },
+			std::tuple { "-", plus_minus_operator<std::minus<>> },
+			std::tuple { "*", binary_operator<std::multiplies<>, '*'> },
+			std::tuple { "/", binary_operator<std::divides<>, '/'> },
 
-		operators["+"] = plus_minus_operator<std::plus<>>;
-		operators["-"] = plus_minus_operator<std::minus<>>;
-		operators["*"] = binary_operator<std::multiplies<>, '*'>;
-		operators["/"] = binary_operator<std::divides<>, '/'>;
+			std::tuple { "<",  comparison_operator<std::less<>> },
+			std::tuple { ">",  comparison_operator<std::greater<>> },
+			std::tuple { "<=", comparison_operator<std::less_equal<>> },
+			std::tuple { ">=", comparison_operator<std::greater_equal<>> },
 
-		operators["<"]  = comparison_operator<std::less<>>;
-		operators[">"]  = comparison_operator<std::greater<>>;
-		operators["<="] = comparison_operator<std::less_equal<>>;
-		operators[">="] = comparison_operator<std::greater_equal<>>;
+			std::tuple { "==", equality_operator<std::equal_to<>> },
+			std::tuple { "!=", equality_operator<std::not_equal_to<>> },
 
-		operators["=="] = equality_operator<std::equal_to<>>;
-		operators["!="] = equality_operator<std::not_equal_to<>>;
+			std::tuple { ".",
+				+[](Interpreter &i, std::vector<Value> args) -> Result<Value> {
+					assert(args.size() == 2, "Operator . requires two arguments"); // TODO(assert)
+					assert(args.back().type == Value::Type::Number, "Only numbers can be used for indexing"); // TODO(assert)
+					return std::move(args.front()).index(i, std::move(args.back()).n.as_int());
+				}
+			},
 
-		operators["."] = +[](Interpreter &i, std::vector<Value> args) -> Result<Value> {
-			assert(args.size() == 2, "Operator . requires two arguments"); // TODO(assert)
-			assert(args.back().type == Value::Type::Number, "Only numbers can be used for indexing"); // TODO(assert)
-			return std::move(args.front()).index(i, std::move(args.back()).n.as_int());
-		};
+			std::tuple { "&",
+				+[](Interpreter&, std::vector<Value> args) -> Result<Value> {
+					using Chord_Chord = Shape<Value::Type::Music, Value::Type::Music>;
 
-		operators["&"] = +[](Interpreter &i, std::vector<Value> args) -> Result<Value> {
-			using Chord_Chord = Shape<Value::Type::Music, Value::Type::Music>;
+					if (Chord_Chord::typecheck(args)) {
+						auto [lhs, rhs] = Chord_Chord::move_from(args);
+						auto &l = lhs.notes;
+						auto &r = rhs.notes;
 
-			if (Chord_Chord::typecheck(args)) {
-				auto [lhs, rhs] = Chord_Chord::move_from(args);
-				auto &l = lhs.notes;
-				auto &r = rhs.notes;
+						// Append one set of notes to another to make bigger chord!
+						l.reserve(l.size() + r.size());
+						std::move(r.begin(), r.end(), std::back_inserter(l));
 
-				// Append one set of notes to another to make bigger chord!
-				l.reserve(l.size() + r.size());
-				std::move(r.begin(), r.end(), std::back_inserter(l));
-
-				return Value::from(lhs);
-			}
-
-			return Error {
-				.details = errors::Unsupported_Types_For {
-					.type = errors::Unsupported_Types_For::Operator,
-					.name = "&",
-					.possibilities = {
-						"(music, music) -> music",
+						return Value::from(lhs);
 					}
-				},
-				.location = {}
-			};
+
+					return Error {
+						.details = errors::Unsupported_Types_For {
+							.type = errors::Unsupported_Types_For::Operator,
+							.name = "&",
+							.possibilities = {
+								"(music, music) -> music",
+							}
+						},
+						.location = {}
+					};
+				}
+			},
 		};
+
+		// All operators should be defined here except 'and' and 'or' which handle evaluation differently
+		// and are need unevaluated expressions for their proper evaluation. Exclusion of them is marked
+		// as subtraction of total excluded operators from expected constant
+		static_assert(Operators.size() == Operators_Count - 2, "All operators handlers are defined here");
+
+		// Set all predefined operators into operators array
+		for (auto &[name, fptr] : Operators) { operators[name] = fptr; }
 	}
 }
 
diff --git a/src/musique.hh b/src/musique.hh
index 70ae4eb..4b7f83a 100644
--- a/src/musique.hh
+++ b/src/musique.hh
@@ -404,7 +404,8 @@ struct Token
 	Location location;
 };
 
-static constexpr auto Keywords_Count = 6;
+static constexpr usize Keywords_Count  =  6;
+static constexpr usize Operators_Count = 14;
 
 std::string_view type_name(Token::Type type);
 
diff --git a/src/parser.cc b/src/parser.cc
index f4f125f..f276b69 100644
--- a/src/parser.cc
+++ b/src/parser.cc
@@ -498,6 +498,12 @@ constexpr bool one_of(std::string_view id, auto const& ...args)
 
 static usize precedense(std::string_view op)
 {
+	// Operators that are not included below are
+	//  '.' since it have own precedense rules and is not binary expression but its own kind of expression
+	//
+	//  Exclusion of them is marked by subtracting total number of excluded operators.
+	static_assert(Operators_Count - 1 == 13, "Ensure that all operators have defined precedense below");
+
 	if (one_of(op, "or")) return 100;
 	if (one_of(op, "and")) return 150;
 	if (one_of(op, "<", ">", "<=", ">=", "==", "!=")) return 200;