From b124b762587884fd1f22884c7893fe3998507707 Mon Sep 17 00:00:00 2001
From: Alex Ames <amablue@google.com>
Date: Mon, 22 Jun 2020 17:02:15 -0700
Subject: [PATCH] Removed requirement that enums be declared in ascending
 order. (#5887)

---
 src/idl_parser.cpp | 29 +++++++++++++----------------
 tests/test.cpp     |  8 ++++----
 2 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/src/idl_parser.cpp b/src/idl_parser.cpp
index a77100a81..ae874cfab 100644
--- a/src/idl_parser.cpp
+++ b/src/idl_parser.cpp
@@ -2000,21 +2000,16 @@ struct EnumValBuilder {
   FLATBUFFERS_CHECKED_ERROR AssignEnumeratorValue(const std::string &value) {
     user_value = true;
     auto fit = false;
-    auto ascending = false;
     if (enum_def.IsUInt64()) {
       uint64_t u64;
       fit = StringToNumber(value.c_str(), &u64);
-      ascending = u64 > temp->GetAsUInt64();
       temp->value = static_cast<int64_t>(u64);  // well-defined since C++20.
     } else {
       int64_t i64;
       fit = StringToNumber(value.c_str(), &i64);
-      ascending = i64 > temp->GetAsInt64();
       temp->value = i64;
     }
     if (!fit) return parser.Error("enum value does not fit, \"" + value + "\"");
-    if (!ascending && strict_ascending && !enum_def.vals.vec.empty())
-      return parser.Error("enum values must be specified in ascending order");
     return NoError();
   }
 
@@ -2050,11 +2045,10 @@ struct EnumValBuilder {
     return parser.Error("fatal: invalid enum underlying type");
   }
 
-  EnumValBuilder(Parser &_parser, EnumDef &_enum_def, bool strict_order = true)
+  EnumValBuilder(Parser &_parser, EnumDef &_enum_def)
       : parser(_parser),
         enum_def(_enum_def),
         temp(nullptr),
-        strict_ascending(strict_order),
         user_value(false) {}
 
   ~EnumValBuilder() { delete temp; }
@@ -2062,7 +2056,6 @@ struct EnumValBuilder {
   Parser &parser;
   EnumDef &enum_def;
   EnumVal *temp;
-  const bool strict_ascending;
   bool user_value;
 };
 
@@ -2099,9 +2092,7 @@ CheckedError Parser::ParseEnum(const bool is_union, EnumDef **dest) {
     // todo: Convert to the Error in the future?
     Warning("underlying type of bit_flags enum must be unsigned");
   }
-  // Protobuf allows them to be specified in any order, so sort afterwards.
-  const auto strict_ascending = (false == opts.proto_mode);
-  EnumValBuilder evb(*this, *enum_def, strict_ascending);
+  EnumValBuilder evb(*this, *enum_def);
   EXPECT('{');
   // A lot of code generatos expect that an enum is not-empty.
   if ((is_union || Is('}')) && !opts.proto_mode) {
@@ -2145,9 +2136,6 @@ CheckedError Parser::ParseEnum(const bool is_union, EnumDef **dest) {
         NEXT();
         ECHECK(evb.AssignEnumeratorValue(attribute_));
         EXPECT(kTokenIntegerConstant);
-      } else if (false == strict_ascending) {
-        // The opts.proto_mode flag is active.
-        return Error("Protobuf mode doesn't allow implicit enum values.");
       }
 
       ECHECK(evb.AcceptEnumerator());
@@ -2183,8 +2171,17 @@ CheckedError Parser::ParseEnum(const bool is_union, EnumDef **dest) {
     }
   }
 
-  if (false == strict_ascending)
-    enum_def->SortByValue();  // Must be sorted to use MinValue/MaxValue.
+  enum_def->SortByValue();  // Must be sorted to use MinValue/MaxValue.
+
+  // Ensure enum value uniqueness.
+  auto prev_it = enum_def->Vals().begin();
+  for (auto it = prev_it + 1; it != enum_def->Vals().end(); ++it) {
+    auto prev_ev = *prev_it;
+    auto ev = *it;
+    if (prev_ev->GetAsUInt64() == ev->GetAsUInt64())
+      return Error("all enum values must be unique: " + prev_ev->name +
+                   " and " + ev->name + " are both " +
+                   NumToString(ev->GetAsInt64()));  }
 
   if (dest) *dest = enum_def;
   types_.Add(current_namespace_->GetFullyQualifiedName(enum_def->name),
diff --git a/tests/test.cpp b/tests/test.cpp
index cc88be8a8..469687a52 100644
--- a/tests/test.cpp
+++ b/tests/test.cpp
@@ -1605,7 +1605,7 @@ void ErrorTest() {
   TestError("enum X:byte { Y } enum X {", "enum already");
   TestError("enum X:float {}", "underlying");
   TestError("enum X:byte { Y, Y }", "value already");
-  TestError("enum X:byte { Y=2, Z=1 }", "ascending");
+  TestError("enum X:byte { Y=2, Z=2 }", "unique");
   TestError("table X { Y:int; } table X {", "datatype already");
   TestError("struct X (force_align: 7) { Y:int; }", "force_align");
   TestError("struct X {}", "size 0");
@@ -1757,9 +1757,7 @@ void EnumOutOfRangeTest() {
   TestError("enum X:ubyte { Y = -1 }", "enum value does not fit");
   TestError("enum X:ubyte { Y = 256 }", "enum value does not fit");
   TestError("enum X:ubyte { Y = 255, Z }", "enum value does not fit");
-  // Unions begin with an implicit "NONE = 0".
-  TestError("table Y{} union X { Y = -1 }",
-            "enum values must be specified in ascending order");
+  TestError("table Y{} union X { Y = -1 }", "enum value does not fit");
   TestError("table Y{} union X { Y = 256 }", "enum value does not fit");
   TestError("table Y{} union X { Y = 255, Z:Y }", "enum value does not fit");
   TestError("enum X:int { Y = -2147483649 }", "enum value does not fit");
@@ -1802,6 +1800,8 @@ void EnumValueTest() {
           18446744073709551615ULL);
   // Assign non-enum value to enum field. Is it right?
   TEST_EQ(TestValue<int>("{ Y:7 }", "E", "enum E:int { V = 0 }"), 7);
+  // Check that non-ascending values are valid.
+  TEST_EQ(TestValue<int>("{ Y:5 }", "E=V", "enum E:int { Z=10, V=5 }"), 5);
 }
 
 void IntegerOutOfRangeTest() {