From a8d258533087f5aca38d91d366f353089ae2d078 Mon Sep 17 00:00:00 2001
From: Max Brunsfeld <maxbrunsfeld@gmail.com>
Date: Thu, 17 Dec 2015 15:19:58 -0800
Subject: [PATCH] Fix resolution of shift-extra vs reduce actions

---
 .../parse_conflict_manager_spec.cc            | 33 ++++++++++++++++---
 .../build_tables/parse_conflict_manager.cc    | 19 ++++++-----
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/spec/compiler/build_tables/parse_conflict_manager_spec.cc b/spec/compiler/build_tables/parse_conflict_manager_spec.cc
index e92e417c..9f3163b8 100644
--- a/spec/compiler/build_tables/parse_conflict_manager_spec.cc
+++ b/spec/compiler/build_tables/parse_conflict_manager_spec.cc
@@ -41,15 +41,38 @@ describe("ParseConflictManager", []() {
     });
 
     describe("shift-extra actions", [&]() {
-      ParseAction shift_extra = ParseAction::Error();
-      ParseAction other = ParseAction::Shift(2, { 0, 0 });
+      ParseAction shift_extra = ParseAction::ShiftExtra();
+      ParseAction shift = ParseAction::Shift(2, { 0, 0 });
+      ParseAction reduce = ParseAction::Reduce(sym2, 1, -1, AssociativityRight, production);
 
-      it("favors other actions over shift-extra actions", [&]() {
-        result = conflict_manager->resolve(other, shift_extra);
+      it("favors any shift action over a shift-extra actions", [&]() {
+        result = conflict_manager->resolve(shift, shift_extra);
         AssertThat(result.first, IsTrue());
         AssertThat(result.second, Equals(ConflictTypeNone));
 
-        result = conflict_manager->resolve(shift_extra, other);
+        result = conflict_manager->resolve(shift_extra, shift);
+        AssertThat(result.first, IsFalse());
+        AssertThat(result.second, Equals(ConflictTypeNone));
+      });
+
+      it("favors any reduce action over a shift-extra actions", [&]() {
+        result = conflict_manager->resolve(reduce, shift_extra);
+        AssertThat(result.first, IsTrue());
+        AssertThat(result.second, Equals(ConflictTypeNone));
+
+        result = conflict_manager->resolve(shift_extra, reduce);
+        AssertThat(result.first, IsFalse());
+        AssertThat(result.second, Equals(ConflictTypeNone));
+      });
+    });
+
+    describe("reduce-extra actions", [&]() {
+      it("favors shift-extra actions over reduce-extra actions", [&]() {
+        result = conflict_manager->resolve(ParseAction::ShiftExtra(), ParseAction::ReduceExtra(sym1));
+        AssertThat(result.first, IsTrue());
+        AssertThat(result.second, Equals(ConflictTypeNone));
+
+        result = conflict_manager->resolve(ParseAction::ReduceExtra(sym1), ParseAction::ShiftExtra());
         AssertThat(result.first, IsFalse());
         AssertThat(result.second, Equals(ConflictTypeNone));
       });
diff --git a/src/compiler/build_tables/parse_conflict_manager.cc b/src/compiler/build_tables/parse_conflict_manager.cc
index b93b1f5e..52caf305 100644
--- a/src/compiler/build_tables/parse_conflict_manager.cc
+++ b/src/compiler/build_tables/parse_conflict_manager.cc
@@ -21,9 +21,11 @@ pair<bool, ConflictType> ParseConflictManager::resolve(
       return { true, ConflictTypeNone };
 
     case ParseActionTypeShift:
-      if (new_action.type == ParseActionTypeReduce) {
-        if (new_action.extra)
-          return { false, ConflictTypeNone };
+      if (new_action.extra) {
+        return {false, ConflictTypeNone};
+      } else if (old_action.extra) {
+        return {true, ConflictTypeNone};
+      } else if (new_action.type == ParseActionTypeReduce) {
         int min_precedence = old_action.precedence_range.min;
         int max_precedence = old_action.precedence_range.max;
         int new_precedence = new_action.precedence_range.max;
@@ -48,15 +50,14 @@ pair<bool, ConflictType> ParseConflictManager::resolve(
           return { false, ConflictTypeUnresolved };
         }
       }
+      break;
 
     case ParseActionTypeReduce:
-      if (new_action.extra)
-        return { false, ConflictTypeNone };
-      if (old_action.extra)
-        return { true, ConflictTypeNone };
-      if (new_action.extra)
-        return { false, ConflictTypeNone };
       if (new_action.type == ParseActionTypeReduce) {
+        if (new_action.extra)
+          return { false, ConflictTypeNone };
+        if (old_action.extra)
+          return { true, ConflictTypeNone };
         int old_precedence = old_action.precedence_range.min;
         int new_precedence = new_action.precedence_range.min;
         if (new_precedence > old_precedence) {