From 9b60afe6e6f5b6740bf4966d93fa618fd84a7697 Mon Sep 17 00:00:00 2001
From: ManDude <7569514+ManDude@users.noreply.github.com>
Date: Mon, 25 Sep 2023 23:49:12 +0100
Subject: [PATCH] [compiler] fix `compile_condition` not attempting macro
 expansion (#3030)

Ancient bug! Resulted in a 1%-2% decrease in code object file size (and
more importantly the asm is more readable).

Fixes #3029
---
 goalc/compiler/compilation/ControlFlow.cpp | 14 +++++++++-----
 goalc/compiler/compilation/Macro.cpp       | 21 +++++++++++----------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/goalc/compiler/compilation/ControlFlow.cpp b/goalc/compiler/compilation/ControlFlow.cpp
index e900d90a67..08ab604408 100644
--- a/goalc/compiler/compilation/ControlFlow.cpp
+++ b/goalc/compiler/compilation/ControlFlow.cpp
@@ -40,21 +40,25 @@ Condition Compiler::compile_condition(const goos::Object& condition, Env* env, b
       {">", ConditionKind::GT},           {"<", ConditionKind::LT},
       {">=", ConditionKind::GEQ},         {"<=", ConditionKind::LEQ}};
 
+  // we may have gotten a macro as a condition, expand it first.
+  // note : use the `condition` arg for errors still so that the user can actually tell what code
+  // went wrong!
+  auto new_c = expand_macro_completely(condition, env);
+
   // possibly a form with an optimizable condition?
-  if (condition.is_pair()) {
-    auto first = pair_car(condition);
-    auto rest = pair_cdr(condition);
+  if (new_c.is_pair()) {
+    auto& first = pair_car(new_c);
+    auto& rest = pair_cdr(new_c);
 
     if (first.is_symbol()) {
       auto fas = first.as_symbol();
 
       // if there's a not, we can just try again to get an optimization with the invert flipped.
       if (fas->name == "not") {
-        auto arg = pair_car(rest);
         if (!pair_cdr(rest).is_empty_list()) {
           throw_compiler_error(condition, "A condition with \"not\" can have only one argument");
         }
-        return compile_condition(arg, env, !invert);
+        return compile_condition(pair_car(rest), env, !invert);
       }
 
       auto& conditions = invert ? conditions_inverted : conditions_normal;
diff --git a/goalc/compiler/compilation/Macro.cpp b/goalc/compiler/compilation/Macro.cpp
index 9db18a99a3..309983ffa9 100644
--- a/goalc/compiler/compilation/Macro.cpp
+++ b/goalc/compiler/compilation/Macro.cpp
@@ -17,7 +17,7 @@ bool Compiler::try_getting_macro_from_goos(const goos::Object& macro_name, goos:
     got_macro = true;
   }
 
-  if (got_macro) {
+  if (got_macro && dest) {
     *dest = macro_obj;
   }
   return got_macro;
@@ -266,15 +266,16 @@ Val* Compiler::compile_mlet(const goos::Object& form, const goos::Object& rest,
 }
 
 Val* Compiler::compile_macro_expand(const goos::Object& form, const goos::Object& rest, Env* env) {
-  auto macro = pair_car(rest);
-  goos::Object macro_obj;
-  if (!try_getting_macro_from_goos(pair_car(macro), &macro_obj)) {
-    throw_compiler_error(form, "{} is not a macro.", pair_car(macro).print());
+  auto& macro = pair_car(rest);
+  auto& macro_name = pair_car(macro);
+  if (!try_getting_macro_from_goos(macro_name, nullptr)) {
+    throw_compiler_error(form, "invalid argument to `macro-expand`: {} does not exist as a macro",
+                         macro_name.print());
   }
-  // the pretty printer doesn't support macro objects, so we use the pair
-  auto result = goos::Object(macro);
-  while (expand_macro_once(result, &result, env)) {
+  if (!pair_cdr(rest).is_empty_list()) {
+    throw_compiler_error(form, "too many arguments to `macro-expand`");
   }
+  auto result = expand_macro_completely(macro, env);
   auto code = pretty_print::to_string(result);
   lg::print("{}\n", code);
   return get_none();
@@ -285,8 +286,8 @@ bool Compiler::expand_macro_once(const goos::Object& src, goos::Object* out, Env
     return false;
   }
 
-  auto first = src.as_pair()->car;
-  auto rest = src.as_pair()->cdr;
+  auto& first = src.as_pair()->car;
+  auto& rest = src.as_pair()->cdr;
   if (!first.is_symbol()) {
     return false;
   }