Implement binary mode (matching fields and enum values by number)

comparison.cpp

@@ -1,6 +1,7 @@
 #include "comparison.h"
 
 #include <iostream>
+#include <string>
 
 using namespace std;
 
@@ -10,6 +11,9 @@ string Comparison::Item::message() const
 
     switch (type)
     {
+    case Enum_Value_Name_Changed:
+        msg = "Value name changed";
+        break;
     case Enum_Value_Id_Changed:
         msg = "Value ID changed";
         break;
@@ -152,6 +156,10 @@ void Comparison::Section::print(int level)
     }
 }
 
+Comparison::Comparison(const Options & options):
+    options(options)
+{}
+
 bool Comparison::compare_default_value(const FieldDescriptor * field1, const FieldDescriptor * field2)
 {
     if (field1->has_default_value() != field2->has_default_value())
@@ -200,7 +208,9 @@ Comparison::Section * Comparison::compare(const EnumDescriptor * enum1, const En
     for (int i = 0; i < enum1->value_count(); ++i)
     {
         auto * value1 = enum1->value(i);
-        auto * value2 = enum2->FindValueByName(value1->name());
+        auto * value2 = options.binary ?
+                    enum2->FindValueByNumber(value1->number()) :
+                    enum2->FindValueByName(value1->name());
 
         if (value2)
         {
@@ -211,20 +221,30 @@ Comparison::Section * Comparison::compare(const EnumDescriptor * enum1, const En
                 subsection.add_item(Enum_Value_Id_Changed,
                                     to_string(value1->number()), to_string(value2->number()));
             }
+            if (value1->name() != value2->name())
+            {
+                subsection.add_item(Enum_Value_Name_Changed,
+                                    value1->name(), value2->name());
+            }
         }
         else
         {
-            section.add_item(Enum_Value_Removed, value1->name(), "");
+            string value1_id = options.binary ? to_string(value1->number()) : value1->name();
+            section.add_item(Enum_Value_Removed, value1_id, "");
         }
     }
 
     for (int i = 0; i < enum2->value_count(); ++i)
     {
         auto * value2 = enum2->value(i);
-        auto * value1 = enum1->FindValueByName(value2->name());
+        auto * value1 = options.binary ?
+                    enum1->FindValueByNumber(value2->number()) :
+                    enum1->FindValueByName(value2->name());
+
         if (!value1)
         {
-            section.add_item(Enum_Value_Added, "", value2->name());
+            string value2_id = options.binary ? to_string(value2->number()) : value2->name();
+            section.add_item(Enum_Value_Added, "", value2_id);
         }
     }
 
@@ -307,61 +327,36 @@ Comparison::Section * Comparison::compare(const Descriptor * desc1, const Descri
     for (int i = 0; i < desc1->field_count(); ++i)
     {
         auto * field1 = desc1->field(i);
-        auto * field2 = desc2->FindFieldByName(field1->name());
+        auto * field2 = options.binary ?
+                    desc2->FindFieldByNumber(field1->number()) :
+                    desc2->FindFieldByName(field1->name());
 
         if (field2)
         {
             section.subsections.push_back(compare(field1, field2));
         }
         else
         {
-            section.add_item(Message_Field_Removed, field1->name(), "");
+            string field1_id = options.binary ? to_string(field1->number()) : field1->name();
+            section.add_item(Message_Field_Removed, field1_id, "");
         }
     }
 
     for (int i = 0; i < desc2->field_count(); ++i)
     {
         auto * field2 = desc2->field(i);
-        auto * field1 = desc1->FindFieldByName(field2->name());
+        auto * field1 =  options.binary ?
+                    desc1->FindFieldByNumber(field2->number()) :
+                    desc1->FindFieldByName(field2->name());
+
         if (!field1)
         {
-            section.add_item(Message_Field_Added, "", field2->name());
+            string field2_id = options.binary ? to_string(field2->number()) : field2->name();
+            section.add_item(Message_Field_Added, "", field2_id);
         }
     }
 
     return &section;
-
-#if 0
-    //
-    std::set<int> field_number_set;
-
-    for (int i = 0; i < desc1.field_count(); ++i)
-    {
-        field_number_set.insert(desc1.field(i)->number());
-    }
-
-    for (int i = 0; i < desc2.field_count(); ++i)
-    {
-        field_number_set.insert(desc2.field(i)->number());
-    }
-
-    for (int field_number : field_number_set)
-    {
-        auto field1 = desc1->FindFieldByNumber(field_number);
-        auto field2 = desc2->FindFieldByNumber(field_number);
-        if (!field1)
-        {
-            cout << desc2->full_name() << ": added field id: " << field_number << endl;
-            continue;
-        }
-        if (!field2)
-        {
-            cout << desc2->full_name() << ": removed field id: " << field_number << endl;
-            continue;
-        }
-        compare(field1, field2);
-    }
-#endif
 }
 
 void Comparison::compare(Source & source1, Source & source2)

comparison.h

@@ -73,6 +73,7 @@ class Comparison
 public:
     enum ItemType
     {
+        Enum_Value_Name_Changed,
         Enum_Value_Id_Changed,
         Enum_Value_Added,
         Enum_Value_Removed,
@@ -153,6 +154,14 @@ class Comparison
         void print(int level = 0);
     };
 
+    struct Options
+    {
+        Options() {}
+        bool binary = false;
+    };
+
+    Comparison(const Options & options = Options{});
+
     void compare(Source & source1, Source & source2);
     void compare(Source & source1, const string & name1, Source & source2, const string &name2);
     Section * compare(const EnumDescriptor * enum1, const EnumDescriptor * enum2);
@@ -163,4 +172,7 @@ class Comparison
     Section root { Root_Section, "", "" };
 
     unordered_map<string, Section*> compared;
+
+private:
+    Options options;
 };

main.cpp

@@ -8,12 +8,31 @@ int main(int argc, char * argv[])
 {
     if (argc < 6)
     {
-        cerr << "Expected arguments: root-dir1 file1 root-dir2 file2 message" << endl;
-        cerr << "Use '.' for message to compare all messages in given files." << endl;
+        cerr << "Expected arguments: root-dir1 file1 root-dir2 file2 type [--binary]" << endl;
+        cerr << "Use '.' for <type> to compare all messages and enums in given files." << endl;
         return 1;
     }
 
-    Comparison comparison;
+    Comparison::Options options;
+
+    if (argc > 6)
+    {
+        for (int i = 6; i < argc; ++i)
+        {
+            string arg = argv[i];
+            if (arg == "--binary")
+            {
+                options.binary = true;
+            }
+            else
+            {
+                cerr << "Unknown option: " << arg << endl;
+                return 1;
+            }
+        }
+    }
+
+    Comparison comparison(options);
 
     try
     {

tests/CMakeLists.txt

@@ -2,13 +2,17 @@
 add_executable(run-tests test.cpp ../comparison.cpp)
 target_link_libraries(run-tests protoc protobuf)
 
-function(add_comparison_test dir_name)
-  message(STATUS "Adding test ${dir_name}")
+function(add_comparison_test_w_options dir_name options)
+  message(STATUS "Adding test ${dir_name} ${options}")
   add_test(NAME "${dir_name}" COMMAND
-          run-tests "${dir_name}"
+          run-tests "${dir_name}" ${options}
           WORKING_DIRECTORY "${CMAKE_CURRENT_SOURCE_DIR}")
 endfunction()
 
+function(add_comparison_test dir_name)
+  add_comparison_test_w_options("${dir_name}" "")
+endfunction()
+
 add_comparison_test(msg_added)
 add_comparison_test(msg_removed)
 add_comparison_test(enum_added)
@@ -27,3 +31,5 @@ add_comparison_test(field_message_type_changed)
 add_comparison_test(field_enum_type_name_changed)
 add_comparison_test(field_enum_type_changed)
 add_comparison_test(msg_recursion)
+add_comparison_test_w_options(binary_message_diff --binary)
+add_comparison_test_w_options(binary_enum_diff --binary)

tests/binary_enum_diff/a.proto

@@ -0,0 +1,8 @@
+syntax = "proto2";
+
+package Test;
+
+enum E {
+  v1 = 1;
+  v2 = 2;
+}

tests/binary_enum_diff/b.proto

@@ -0,0 +1,8 @@
+syntax = "proto2";
+
+package Test;
+
+enum E {
+  v2_changed = 2;
+  v3 = 3;
+}

tests/binary_enum_diff/diff.json

@@ -0,0 +1,27 @@
+{
+  "type": "/",
+  "sections": [{
+    "type": "enum_comparison",
+    "a": "Test.E",
+    "b": "Test.E",
+    "items": [{
+      "type": "enum_value_removed",
+      "a": "1",
+      "b": ""
+    },{
+      "type": "enum_value_added",
+      "a": "",
+      "b": "3"
+    }],
+    "sections": [{
+      "type": "enum_value_comparison",
+      "a": "v2",
+      "b": "v2_changed",
+      "items": [{
+        "type": "enum_value_name_changed",
+        "a": "v2",
+        "b": "v2_changed"
+      }]
+    }]
+  }]
+}

tests/binary_message_diff/a.proto

@@ -0,0 +1,9 @@
+syntax = "proto2";
+
+package Test;
+
+message M {
+  optional float f1 = 1;
+  optional float f2 = 2;
+}
+

tests/binary_message_diff/b.proto

@@ -0,0 +1,8 @@
+syntax = "proto2";
+
+package Test;
+
+message M {
+  optional bool f2_changed = 2;
+  optional float f3 = 3;
+}

tests/binary_message_diff/diff.json

@@ -0,0 +1,31 @@
+{
+  "type": "/",
+  "sections": [{
+    "type": "message_comparison",
+    "a": "Test.M",
+    "b": "Test.M",
+    "items": [{
+      "type": "message_field_removed",
+      "a": "1",
+      "b": ""
+    },{
+      "type": "message_field_added",
+      "a": "",
+      "b": "3"
+    }],
+    "sections": [{
+      "type": "message_field_comparison",
+      "a": "f2",
+      "b": "f2_changed",
+      "items": [{
+        "type": "message_field_name_changed",
+        "a": "f2",
+        "b": "f2_changed"
+      },{
+        "type": "message_field_type_changed",
+        "a": "float",
+        "b": "bool"
+      }]
+    }]
+  }]
+}

tests/test.cpp

@@ -38,6 +38,8 @@ string item_type_string(Comparison::ItemType type)
 {
     switch (type)
     {
+    case Comparison::Enum_Value_Name_Changed:
+        return "enum_value_name_changed";
     case Comparison::Enum_Value_Id_Changed:
         return "enum_value_id_changed";
     case Comparison::Enum_Value_Added:
@@ -153,7 +155,26 @@ int main(int argc, char * argv[])
 
     string test_path(argv[1]);
 
-    Comparison comparison;
+    Comparison::Options options;
+
+    if (argc > 2)
+    {
+        for (int i = 2; i < argc; ++i)
+        {
+            string arg = argv[i];
+            if (arg == "--binary")
+            {
+                options.binary = true;
+            }
+            else
+            {
+                cerr << "Unknown option: " << arg << endl;
+                return 1;
+            }
+        }
+    }
+
+    Comparison comparison(options);
 
     try
     {
@@ -167,6 +188,10 @@ int main(int argc, char * argv[])
         return 1;
     }
 
+    comparison.root.trim();
+
+    comparison.root.print();
+
     json expected;
 
     try
@@ -187,8 +212,6 @@ int main(int argc, char * argv[])
         return 1;
     }
 
-    comparison.root.trim();
-
     try
     {
         verify(comparison, expected);