Merge pull request #7663 from jordan-woyak/expression-parser-improve

Expression parser improvements

1 files changed, 479 insertions, 302 deletions

diff --git a/Source/Core/InputCommon/ControlReference/ExpressionParser.cpp b/Source/Core/InputCommon/ControlReference/ExpressionParser.cpp
index c864b22a20..00afc5e036 100644
--- a/Source/Core/InputCommon/ControlReference/ExpressionParser.cpp
+++ b/Source/Core/InputCommon/ControlReference/ExpressionParser.cpp
@@ -2,222 +2,201 @@
 // Licensed under GPLv2+
 // Refer to the license.txt file included.
 
-#include <algorithm>
 #include <cassert>
+#include <cmath>
 #include <iostream>
-#include <map>
 #include <memory>
+#include <regex>
 #include <string>
+#include <utility>
 #include <vector>
 
+#include "Common/Common.h"
 #include "Common/StringUtil.h"
+
 #include "InputCommon/ControlReference/ExpressionParser.h"
+#include "InputCommon/ControlReference/FunctionExpression.h"
 
 namespace ciface::ExpressionParser
 {
 using namespace ciface::Core;
 
-enum TokenType
-{
-  TOK_DISCARD,
-  TOK_INVALID,
-  TOK_EOF,
-  TOK_LPAREN,
-  TOK_RPAREN,
-  TOK_AND,
-  TOK_OR,
-  TOK_NOT,
-  TOK_ADD,
-  TOK_CONTROL,
-};
+Token::Token(TokenType type_) : type(type_)
+{
+}
 
-inline std::string OpName(TokenType op)
+Token::Token(TokenType type_, std::string data_) : type(type_), data(std::move(data_))
 {
-  switch (op)
-  {
-  case TOK_AND:
-    return "And";
-  case TOK_OR:
-    return "Or";
-  case TOK_NOT:
-    return "Not";
-  case TOK_ADD:
-    return "Add";
-  default:
-    assert(false);
-    return "";
-  }
 }
 
-class Token
+bool Token::IsBinaryOperator() const
 {
-public:
-  TokenType type;
-  ControlQualifier qualifier;
+  return type >= TOK_BINARY_OPS_BEGIN && type < TOK_BINARY_OPS_END;
+}
 
-  Token(TokenType type_) : type(type_) {}
-  Token(TokenType type_, ControlQualifier qualifier_) : type(type_), qualifier(qualifier_) {}
-  operator std::string() const
-  {
-    switch (type)
-    {
-    case TOK_DISCARD:
-      return "Discard";
-    case TOK_EOF:
-      return "EOF";
-    case TOK_LPAREN:
-      return "(";
-    case TOK_RPAREN:
-      return ")";
-    case TOK_AND:
-      return "&";
-    case TOK_OR:
-      return "|";
-    case TOK_NOT:
-      return "!";
-    case TOK_ADD:
-      return "+";
-    case TOK_CONTROL:
-      return "Device(" + (std::string)qualifier + ")";
-    case TOK_INVALID:
-      break;
-    }
+Lexer::Lexer(std::string expr_) : expr(std::move(expr_))
+{
+  it = expr.begin();
+}
 
-    return "Invalid";
-  }
-};
+std::string Lexer::FetchDelimString(char delim)
+{
+  const std::string result = FetchCharsWhile([delim](char c) { return c != delim; });
+  if (it != expr.end())
+    ++it;
+  return result;
+}
 
-class Lexer
+std::string Lexer::FetchWordChars()
 {
-public:
-  std::string expr;
-  std::string::iterator it;
+  // Valid word characters:
+  std::regex rx(R"([a-z\d_])", std::regex_constants::icase);
 
-  Lexer(const std::string& expr_) : expr(expr_) { it = expr.begin(); }
-  bool FetchBacktickString(std::string& value, char otherDelim = 0)
-  {
-    value = "";
-    while (it != expr.end())
-    {
-      char c = *it;
-      ++it;
-      if (c == '`')
-        return false;
-      if (c > 0 && c == otherDelim)
-        return true;
-      value += c;
-    }
-    return false;
-  }
+  return FetchCharsWhile([&rx](char c) { return std::regex_match(std::string(1, c), rx); });
+}
 
-  Token GetFullyQualifiedControl()
-  {
-    ControlQualifier qualifier;
-    std::string value;
+Token Lexer::GetDelimitedLiteral()
+{
+  return Token(TOK_LITERAL, FetchDelimString('\''));
+}
 
-    if (FetchBacktickString(value, ':'))
-    {
-      // Found colon, this is the device name
-      qualifier.has_device = true;
-      qualifier.device_qualifier.FromString(value);
-      FetchBacktickString(value);
-    }
+Token Lexer::GetVariable()
+{
+  return Token(TOK_VARIABLE, FetchWordChars());
+}
 
-    qualifier.control_name = value;
+Token Lexer::GetFullyQualifiedControl()
+{
+  return Token(TOK_CONTROL, FetchDelimString('`'));
+}
 
-    return Token(TOK_CONTROL, qualifier);
-  }
+Token Lexer::GetBareword(char first_char)
+{
+  return Token(TOK_BAREWORD, first_char + FetchWordChars());
+}
 
-  Token GetBarewordsControl(char c)
-  {
-    std::string name;
-    name += c;
+Token Lexer::GetRealLiteral(char first_char)
+{
+  std::string value;
+  value += first_char;
+  value += FetchCharsWhile([](char c) { return isdigit(c, std::locale::classic()) || ('.' == c); });
 
-    while (it != expr.end())
-    {
-      c = *it;
-      if (!isalpha(c))
-        break;
-      name += c;
-      ++it;
-    }
+  if (std::regex_match(value, std::regex(R"(\d+(\.\d+)?)")))
+    return Token(TOK_LITERAL, value);
 
-    ControlQualifier qualifier;
-    qualifier.control_name = name;
-    return Token(TOK_CONTROL, qualifier);
-  }
+  return Token(TOK_INVALID);
+}
 
-  Token NextToken()
-  {
-    if (it == expr.end())
-      return Token(TOK_EOF);
+Token Lexer::PeekToken()
+{
+  const auto old_it = it;
+  const auto tok = NextToken();
+  it = old_it;
+  return tok;
+}
 
-    char c = *it++;
-    switch (c)
-    {
-    case ' ':
-    case '\t':
-    case '\n':
-    case '\r':
-      return Token(TOK_DISCARD);
-    case '(':
-      return Token(TOK_LPAREN);
-    case ')':
-      return Token(TOK_RPAREN);
-    case '&':
-      return Token(TOK_AND);
-    case '|':
-      return Token(TOK_OR);
-    case '!':
-      return Token(TOK_NOT);
-    case '+':
-      return Token(TOK_ADD);
-    case '`':
-      return GetFullyQualifiedControl();
-    default:
-      if (isalpha(c))
-        return GetBarewordsControl(c);
-      else
-        return Token(TOK_INVALID);
-    }
+Token Lexer::NextToken()
+{
+  if (it == expr.end())
+    return Token(TOK_EOF);
+
+  char c = *it++;
+  switch (c)
+  {
+  case ' ':
+  case '\t':
+  case '\n':
+  case '\r':
+    return Token(TOK_WHITESPACE);
+  case '(':
+    return Token(TOK_LPAREN);
+  case ')':
+    return Token(TOK_RPAREN);
+  case '&':
+    return Token(TOK_AND);
+  case '|':
+    return Token(TOK_OR);
+  case '!':
+    return Token(TOK_NOT);
+  case '+':
+    return Token(TOK_ADD);
+  case '-':
+    return Token(TOK_SUB);
+  case '*':
+    return Token(TOK_MUL);
+  case '/':
+    return Token(TOK_DIV);
+  case '%':
+    return Token(TOK_MOD);
+  case '=':
+    return Token(TOK_ASSIGN);
+  case '<':
+    return Token(TOK_LTHAN);
+  case '>':
+    return Token(TOK_GTHAN);
+  case ',':
+    return Token(TOK_COMMA);
+  case '\'':
+    return GetDelimitedLiteral();
+  case '$':
+    return GetVariable();
+  case '`':
+    return GetFullyQualifiedControl();
+  default:
+    if (isalpha(c, std::locale::classic()))
+      return GetBareword(c);
+    else if (isdigit(c, std::locale::classic()))
+      return GetRealLiteral(c);
+    else
+      return Token(TOK_INVALID);
   }
+}
 
-  ParseStatus Tokenize(std::vector<Token>& tokens)
+ParseStatus Lexer::Tokenize(std::vector<Token>& tokens)
+{
+  while (true)
   {
-    while (true)
-    {
-      Token tok = NextToken();
+    const std::size_t string_position = it - expr.begin();
+    Token tok = NextToken();
 
-      if (tok.type == TOK_DISCARD)
-        continue;
+    tok.string_position = string_position;
+    tok.string_length = it - expr.begin();
 
-      if (tok.type == TOK_INVALID)
-      {
-        tokens.clear();
+    // Handle /* */ style comments.
+    if (tok.type == TOK_DIV && PeekToken().type == TOK_MUL)
+    {
+      const auto end_of_comment = expr.find("*/", it - expr.begin());
+
+      if (end_of_comment == std::string::npos)
         return ParseStatus::SyntaxError;
-      }
 
-      tokens.push_back(tok);
+      tok.type = TOK_COMMENT;
+      tok.string_length = end_of_comment + 4;
 
-      if (tok.type == TOK_EOF)
-        break;
+      it = expr.begin() + end_of_comment + 2;
     }
-    return ParseStatus::Successful;
+
+    tokens.push_back(tok);
+
+    if (tok.type == TOK_INVALID)
+      return ParseStatus::SyntaxError;
+
+    if (tok.type == TOK_EOF)
+      break;
   }
-};
+  return ParseStatus::Successful;
+}
 
 class ControlExpression : public Expression
 {
 public:
-  ControlQualifier qualifier;
-  Device::Control* control = nullptr;
-  // Keep a shared_ptr to the device so the control pointer doesn't become invalid
+  // Keep a shared_ptr to the device so the control pointer doesn't become invalid.
   std::shared_ptr<Device> m_device;
 
   explicit ControlExpression(ControlQualifier qualifier_) : qualifier(qualifier_) {}
   ControlState GetValue() const override
   {
-    if (!control)
+    if (!input)
       return 0.0;
 
     // Note: Inputs may return negative values in situations where opposing directions are
@@ -226,20 +205,25 @@ public:
     // FYI: Clamping values greater than 1.0 is purposely not done to support unbounded values in
     // the future. (e.g. raw accelerometer/gyro data)
 
-    return std::max(0.0, control->ToInput()->GetState());
+    return std::max(0.0, input->GetState());
   }
   void SetValue(ControlState value) override
   {
-    if (control)
-      control->ToOutput()->SetState(value);
+    if (output)
+      output->SetState(value);
   }
-  int CountNumControls() const override { return control ? 1 : 0; }
-  void UpdateReferences(ControlFinder& finder) override
+  int CountNumControls() const override { return (input || output) ? 1 : 0; }
+  void UpdateReferences(ControlEnvironment& env) override
   {
-    m_device = finder.FindDevice(qualifier);
-    control = finder.FindControl(qualifier);
+    m_device = env.FindDevice(qualifier);
+    input = env.FindInput(qualifier);
+    output = env.FindOutput(qualifier);
   }
-  operator std::string() const override { return "`" + static_cast<std::string>(qualifier) + "`"; }
+
+private:
+  ControlQualifier qualifier;
+  Device::Input* input = nullptr;
+  Device::Output* output = nullptr;
 };
 
 class BinaryExpression : public Expression
@@ -257,16 +241,43 @@ public:
 
   ControlState GetValue() const override
   {
-    ControlState lhsValue = lhs->GetValue();
-    ControlState rhsValue = rhs->GetValue();
     switch (op)
     {
     case TOK_AND:
-      return std::min(lhsValue, rhsValue);
+      return std::min(lhs->GetValue(), rhs->GetValue());
     case TOK_OR:
-      return std::max(lhsValue, rhsValue);
+      return std::max(lhs->GetValue(), rhs->GetValue());
     case TOK_ADD:
-      return std::min(lhsValue + rhsValue, 1.0);
+      return lhs->GetValue() + rhs->GetValue();
+    case TOK_SUB:
+      return lhs->GetValue() - rhs->GetValue();
+    case TOK_MUL:
+      return lhs->GetValue() * rhs->GetValue();
+    case TOK_DIV:
+    {
+      const ControlState result = lhs->GetValue() / rhs->GetValue();
+      return std::isinf(result) ? 0.0 : result;
+    }
+    case TOK_MOD:
+    {
+      const ControlState result = std::fmod(lhs->GetValue(), rhs->GetValue());
+      return std::isnan(result) ? 0.0 : result;
+    }
+    case TOK_ASSIGN:
+    {
+      lhs->SetValue(rhs->GetValue());
+      return lhs->GetValue();
+    }
+    case TOK_LTHAN:
+      return lhs->GetValue() < rhs->GetValue();
+    case TOK_GTHAN:
+      return lhs->GetValue() > rhs->GetValue();
+    case TOK_COMMA:
+    {
+      // Eval and discard lhs:
+      lhs->GetValue();
+      return rhs->GetValue();
+    }
     default:
       assert(false);
       return 0;
@@ -286,57 +297,73 @@ public:
     return lhs->CountNumControls() + rhs->CountNumControls();
   }
 
-  void UpdateReferences(ControlFinder& finder) override
+  void UpdateReferences(ControlEnvironment& env) override
   {
-    lhs->UpdateReferences(finder);
-    rhs->UpdateReferences(finder);
-  }
-
-  operator std::string() const override
-  {
-    return OpName(op) + "(" + (std::string)(*lhs) + ", " + (std::string)(*rhs) + ")";
+    lhs->UpdateReferences(env);
+    rhs->UpdateReferences(env);
   }
 };
 
-class UnaryExpression : public Expression
+class LiteralExpression : public Expression
 {
 public:
-  TokenType op;
-  std::unique_ptr<Expression> inner;
-
-  UnaryExpression(TokenType op_, std::unique_ptr<Expression>&& inner_)
-      : op(op_), inner(std::move(inner_))
+  void SetValue(ControlState) override
   {
+    // Do nothing.
   }
-  ControlState GetValue() const override
+
+  int CountNumControls() const override { return 1; }
+
+  void UpdateReferences(ControlEnvironment&) override
   {
-    ControlState value = inner->GetValue();
-    switch (op)
-    {
-    case TOK_NOT:
-      return 1.0 - value;
-    default:
-      assert(false);
-      return 0;
-    }
+    // Nothing needed.
   }
 
-  void SetValue(ControlState value) override
-  {
-    switch (op)
-    {
-    case TOK_NOT:
-      inner->SetValue(1.0 - value);
-      break;
+protected:
+  virtual std::string GetName() const = 0;
+};
 
-    default:
-      assert(false);
-    }
+class LiteralReal : public LiteralExpression
+{
+public:
+  LiteralReal(ControlState value) : m_value(value) {}
+
+  ControlState GetValue() const override { return m_value; }
+
+  std::string GetName() const override { return ValueToString(m_value); }
+
+private:
+  const ControlState m_value{};
+};
+
+ParseResult MakeLiteralExpression(Token token)
+{
+  ControlState val{};
+  if (TryParse(token.data, &val))
+    return ParseResult::MakeSuccessfulResult(std::make_unique<LiteralReal>(val));
+  else
+    return ParseResult::MakeErrorResult(token, _trans("Invalid literal."));
+}
+
+class VariableExpression : public Expression
+{
+public:
+  VariableExpression(std::string name) : m_name(name) {}
+
+  ControlState GetValue() const override { return *m_value_ptr; }
+
+  void SetValue(ControlState value) override { *m_value_ptr = value; }
+
+  int CountNumControls() const override { return 1; }
+
+  void UpdateReferences(ControlEnvironment& env) override
+  {
+    m_value_ptr = env.GetVariablePtr(m_name);
   }
 
-  int CountNumControls() const override { return inner->CountNumControls(); }
-  void UpdateReferences(ControlFinder& finder) override { inner->UpdateReferences(finder); }
-  operator std::string() const override { return OpName(op) + "(" + (std::string)(*inner) + ")"; }
+protected:
+  const std::string m_name;
+  ControlState* m_value_ptr{};
 };
 
 // This class proxies all methods to its either left-hand child if it has bound controls, or its
@@ -353,16 +380,10 @@ public:
   void SetValue(ControlState value) override { GetActiveChild()->SetValue(value); }
 
   int CountNumControls() const override { return GetActiveChild()->CountNumControls(); }
-  operator std::string() const override
+  void UpdateReferences(ControlEnvironment& env) override
   {
-    return "Coalesce(" + static_cast<std::string>(*m_lhs) + ", " +
-           static_cast<std::string>(*m_rhs) + ')';
-  }
-
-  void UpdateReferences(ControlFinder& finder) override
-  {
-    m_lhs->UpdateReferences(finder);
-    m_rhs->UpdateReferences(finder);
+    m_lhs->UpdateReferences(env);
+    m_rhs->UpdateReferences(env);
   }
 
 private:
@@ -375,7 +396,7 @@ private:
   std::unique_ptr<Expression> m_rhs;
 };
 
-std::shared_ptr<Device> ControlFinder::FindDevice(ControlQualifier qualifier) const
+std::shared_ptr<Device> ControlEnvironment::FindDevice(ControlQualifier qualifier) const
 {
   if (qualifier.has_device)
     return container.FindDevice(qualifier.device_qualifier);
@@ -383,150 +404,303 @@ std::shared_ptr<Device> ControlFinder::FindDevice(ControlQualifier qualifier) co
     return container.FindDevice(default_device);
 }
 
-Device::Control* ControlFinder::FindControl(ControlQualifier qualifier) const
+Device::Input* ControlEnvironment::FindInput(ControlQualifier qualifier) const
 {
   const std::shared_ptr<Device> device = FindDevice(qualifier);
   if (!device)
     return nullptr;
 
-  if (is_input)
-    return device->FindInput(qualifier.control_name);
-  else
-    return device->FindOutput(qualifier.control_name);
+  return device->FindInput(qualifier.control_name);
 }
 
-struct ParseResult
+Device::Output* ControlEnvironment::FindOutput(ControlQualifier qualifier) const
 {
-  ParseResult(ParseStatus status_, std::unique_ptr<Expression>&& expr_ = {})
-      : status(status_), expr(std::move(expr_))
-  {
-  }
+  const std::shared_ptr<Device> device = FindDevice(qualifier);
+  if (!device)
+    return nullptr;
 
-  ParseStatus status;
-  std::unique_ptr<Expression> expr;
-};
+  return device->FindOutput(qualifier.control_name);
+}
+
+ControlState* ControlEnvironment::GetVariablePtr(const std::string& name)
+{
+  return &m_variables[name];
+}
+
+ParseResult ParseResult::MakeEmptyResult()
+{
+  ParseResult result;
+  result.status = ParseStatus::EmptyExpression;
+  return result;
+}
+
+ParseResult ParseResult::MakeSuccessfulResult(std::unique_ptr<Expression>&& expr)
+{
+  ParseResult result;
+  result.status = ParseStatus::Successful;
+  result.expr = std::move(expr);
+  return result;
+}
+
+ParseResult ParseResult::MakeErrorResult(Token token, std::string description)
+{
+  ParseResult result;
+  result.status = ParseStatus::SyntaxError;
+  result.token = std::move(token);
+  result.description = std::move(description);
+  return result;
+}
 
 class Parser
 {
 public:
-  explicit Parser(std::vector<Token> tokens_) : tokens(tokens_) { m_it = tokens.begin(); }
-  ParseResult Parse() { return Toplevel(); }
+  explicit Parser(const std::vector<Token>& tokens_) : tokens(tokens_) { m_it = tokens.begin(); }
+  ParseResult Parse()
+  {
+    ParseResult result = ParseToplevel();
+
+    if (ParseStatus::Successful != result.status)
+      return result;
+
+    if (Peek().type == TOK_EOF)
+      return result;
+
+    return ParseResult::MakeErrorResult(Peek(), _trans("Expected EOF."));
+  }
 
 private:
-  std::vector<Token> tokens;
-  std::vector<Token>::iterator m_it;
+  const std::vector<Token>& tokens;
+  std::vector<Token>::const_iterator m_it;
+
+  Token Chew()
+  {
+    const Token tok = Peek();
+    if (TOK_EOF != tok.type)
+      ++m_it;
+    return tok;
+  }
 
-  Token Chew() { return *m_it++; }
   Token Peek() { return *m_it; }
+
   bool Expects(TokenType type)
   {
     Token tok = Chew();
     return tok.type == type;
   }
 
-  ParseResult Atom()
+  ParseResult ParseFunctionArguments(const std::string_view& func_name,
+                                     std::unique_ptr<FunctionExpression>&& func,
+                                     const Token& func_tok)
   {
-    Token tok = Chew();
-    switch (tok.type)
+    std::vector<std::unique_ptr<Expression>> args;
+
+    if (TOK_LPAREN != Peek().type)
     {
-    case TOK_CONTROL:
-      return {ParseStatus::Successful, std::make_unique<ControlExpression>(tok.qualifier)};
-    case TOK_LPAREN:
-      return Paren();
-    default:
-      return {ParseStatus::SyntaxError};
+      // Single argument with no parens (useful for unary ! function)
+      const auto tok = Chew();
+      auto arg = ParseAtom(tok);
+      if (ParseStatus::Successful != arg.status)
+        return arg;
+
+      args.emplace_back(std::move(arg.expr));
     }
-  }
+    else
+    {
+      // Chew the L-Paren
+      Chew();
 
-  bool IsUnaryExpression(TokenType type)
-  {
-    switch (type)
+      // Check for empty argument list:
+      if (TOK_RPAREN == Peek().type)
+      {
+        Chew();
+      }
+      else
+      {
+        while (true)
+        {
+          // Read one argument.
+          // Grab an expression, but stop at comma.
+          auto arg = ParseBinary(BinaryOperatorPrecedence(TOK_COMMA));
+          if (ParseStatus::Successful != arg.status)
+            return arg;
+
+          args.emplace_back(std::move(arg.expr));
+
+          // Right paren is the end of our arguments.
+          const Token tok = Chew();
+          if (TOK_RPAREN == tok.type)
+            break;
+
+          // Comma before the next argument.
+          if (TOK_COMMA != tok.type)
+            return ParseResult::MakeErrorResult(tok, _trans("Expected comma."));
+        };
+      }
+    }
+
+    const auto argument_validation = func->SetArguments(std::move(args));
+
+    if (std::holds_alternative<FunctionExpression::ExpectedArguments>(argument_validation))
     {
-    case TOK_NOT:
-      return true;
-    default:
-      return false;
+      const auto text = std::string(func_name) + '(' +
+                        std::get<FunctionExpression::ExpectedArguments>(argument_validation).text +
+                        ')';
+
+      return ParseResult::MakeErrorResult(func_tok, _trans("Expected arguments: " + text));
     }
+
+    return ParseResult::MakeSuccessfulResult(std::move(func));
   }
 
-  ParseResult Unary()
+  ParseResult ParseAtom(const Token& tok)
   {
-    if (IsUnaryExpression(Peek().type))
+    switch (tok.type)
     {
-      Token tok = Chew();
-      ParseResult result = Atom();
-      if (result.status == ParseStatus::SyntaxError)
-        return result;
-      return {ParseStatus::Successful,
-              std::make_unique<UnaryExpression>(tok.type, std::move(result.expr))};
-    }
+    case TOK_BAREWORD:
+    {
+      auto func = MakeFunctionExpression(tok.data);
+
+      if (!func)
+      {
+        // Invalid function, interpret this as a bareword control.
+        Token control_tok(tok);
+        control_tok.type = TOK_CONTROL;
+        return ParseAtom(control_tok);
+      }
 
-    return Atom();
+      return ParseFunctionArguments(tok.data, std::move(func), tok);
+    }
+    case TOK_CONTROL:
+    {
+      ControlQualifier cq;
+      cq.FromString(tok.data);
+      return ParseResult::MakeSuccessfulResult(std::make_unique<ControlExpression>(cq));
+    }
+    case TOK_NOT:
+    {
+      return ParseFunctionArguments("not", MakeFunctionExpression("not"), tok);
+    }
+    case TOK_LITERAL:
+    {
+      return MakeLiteralExpression(tok);
+    }
+    case TOK_VARIABLE:
+    {
+      return ParseResult::MakeSuccessfulResult(std::make_unique<VariableExpression>(tok.data));
+    }
+    case TOK_LPAREN:
+    {
+      return ParseParens();
+    }
+    case TOK_SUB:
+    {
+      // An atom was expected but we got a subtraction symbol.
+      // Interpret it as a unary minus function.
+      return ParseFunctionArguments("minus", MakeFunctionExpression("minus"), tok);
+    }
+    default:
+    {
+      return ParseResult::MakeErrorResult(tok, _trans("Expected start of expression."));
+    }
+    }
   }
 
-  bool IsBinaryToken(TokenType type)
+  static int BinaryOperatorPrecedence(TokenType type)
   {
     switch (type)
     {
+    case TOK_MUL:
+    case TOK_DIV:
+    case TOK_MOD:
+      return 1;
+    case TOK_ADD:
+    case TOK_SUB:
+      return 2;
+    case TOK_GTHAN:
+    case TOK_LTHAN:
+      return 3;
     case TOK_AND:
+      return 4;
     case TOK_OR:
-    case TOK_ADD:
-      return true;
+      return 5;
+    case TOK_ASSIGN:
+      return 6;
+    case TOK_COMMA:
+      return 7;
     default:
-      return false;
+      assert(false);
+      return 0;
     }
   }
 
-  ParseResult Binary()
+  ParseResult ParseBinary(int precedence = 999)
   {
-    ParseResult result = Unary();
-    if (result.status == ParseStatus::SyntaxError)
-      return result;
+    ParseResult lhs = ParseAtom(Chew());
+
+    if (lhs.status == ParseStatus::SyntaxError)
+      return lhs;
+
+    std::unique_ptr<Expression> expr = std::move(lhs.expr);
 
-    std::unique_ptr<Expression> expr = std::move(result.expr);
-    while (IsBinaryToken(Peek().type))
+    // TODO: handle LTR/RTL associativity?
+    while (Peek().IsBinaryOperator() && BinaryOperatorPrecedence(Peek().type) < precedence)
     {
-      Token tok = Chew();
-      ParseResult unary_result = Unary();
-      if (unary_result.status == ParseStatus::SyntaxError)
+      const Token tok = Chew();
+      ParseResult rhs = ParseBinary(BinaryOperatorPrecedence(tok.type));
+      if (rhs.status == ParseStatus::SyntaxError)
       {
-        return unary_result;
+        return rhs;
       }
 
-      expr = std::make_unique<BinaryExpression>(tok.type, std::move(expr),
-                                                std::move(unary_result.expr));
+      expr = std::make_unique<BinaryExpression>(tok.type, std::move(expr), std::move(rhs.expr));
     }
 
-    return {ParseStatus::Successful, std::move(expr)};
+    return ParseResult::MakeSuccessfulResult(std::move(expr));
   }
 
-  ParseResult Paren()
+  ParseResult ParseParens()
   {
     // lparen already chewed
-    ParseResult result = Toplevel();
+    ParseResult result = ParseToplevel();
     if (result.status != ParseStatus::Successful)
       return result;
 
-    if (!Expects(TOK_RPAREN))
+    const auto rparen = Chew();
+    if (rparen.type != TOK_RPAREN)
     {
-      return {ParseStatus::SyntaxError};
+      return ParseResult::MakeErrorResult(rparen, _trans("Expected closing paren."));
     }
 
     return result;
   }
 
-  ParseResult Toplevel() { return Binary(); }
-};
+  ParseResult ParseToplevel() { return ParseBinary(); }
+};  // namespace ExpressionParser
+
+ParseResult ParseTokens(const std::vector<Token>& tokens)
+{
+  return Parser(tokens).Parse();
+}
 
 static ParseResult ParseComplexExpression(const std::string& str)
 {
   Lexer l(str);
   std::vector<Token> tokens;
-  ParseStatus tokenize_status = l.Tokenize(tokens);
+  const ParseStatus tokenize_status = l.Tokenize(tokens);
   if (tokenize_status != ParseStatus::Successful)
-    return {tokenize_status};
+    return ParseResult::MakeErrorResult(Token(TOK_INVALID), _trans("Tokenizing failed."));
 
-  return Parser(std::move(tokens)).Parse();
+  RemoveInertTokens(&tokens);
+  return ParseTokens(tokens);
+}
+
+void RemoveInertTokens(std::vector<Token>* tokens)
+{
+  tokens->erase(std::remove_if(tokens->begin(), tokens->end(),
+                               [](const Token& tok) {
+                                 return tok.type == TOK_COMMENT || tok.type == TOK_WHITESPACE;
+                               }),
+                tokens->end());
 }
 
 static std::unique_ptr<Expression> ParseBarewordExpression(const std::string& str)
@@ -538,21 +712,24 @@ static std::unique_ptr<Expression> ParseBarewordExpression(const std::string& st
   return std::make_unique<ControlExpression>(qualifier);
 }
 
-std::pair<ParseStatus, std::unique_ptr<Expression>> ParseExpression(const std::string& str)
+ParseResult ParseExpression(const std::string& str)
 {
   if (StripSpaces(str).empty())
-    return std::make_pair(ParseStatus::EmptyExpression, nullptr);
+    return ParseResult::MakeEmptyResult();
 
   auto bareword_expr = ParseBarewordExpression(str);
   ParseResult complex_result = ParseComplexExpression(str);
 
   if (complex_result.status != ParseStatus::Successful)
   {
-    return std::make_pair(complex_result.status, std::move(bareword_expr));
+    // This is a bit odd.
+    // Return the error status of the complex expression with the fallback barewords expression.
+    complex_result.expr = std::move(bareword_expr);
+    return complex_result;
   }
 
-  auto combined_expr = std::make_unique<CoalesceExpression>(std::move(bareword_expr),
-                                                            std::move(complex_result.expr));
-  return std::make_pair(complex_result.status, std::move(combined_expr));
+  complex_result.expr = std::make_unique<CoalesceExpression>(std::move(bareword_expr),
+                                                             std::move(complex_result.expr));
+  return complex_result;
 }
 }  // namespace ciface::ExpressionParser