[GitHub] [thrift] fishy commented on a change in pull request #2307: THRIFT-5337 Go set fields write improvement

[GitBox <gi...@apache.org>]

fishy commented on a change in pull request #2307:
URL: https://github.com/apache/thrift/pull/2307#discussion_r568295448



##########
File path: compiler/cpp/src/thrift/generate/t_go_generator.cc
##########
@@ -1851,6 +1864,65 @@ void t_go_generator::generate_go_struct_writer(ostream& out,
   }
 }
 
+void t_go_generator::generate_go_struct_equals(ostream& out,
+                                               t_struct* tstruct,
+                                               const string& tstruct_name,
+                                               bool is_args_or_result) {
+  if (is_args_or_result) {
+    return;
+  }

Review comment:
       nit: I think we should do this check on the call site instead, and currently there's only one caller of it (Line 1497), so there should be:
   
   ```
   if !is_result && !is_args {
     generate_go_struct_equals(out, tstruct, tstruct_name);
   }
   ```
   
   Semantically this function is called "generate_*", but when one of its arg is passed in true it doesn't generate anything, which feels wrong to me.

##########
File path: compiler/cpp/src/thrift/generate/t_go_generator.cc
##########
@@ -3463,6 +3550,112 @@ void t_go_generator::generate_serialize_list_element(ostream& out, t_list* tlist
   generate_serialize_field(out, &efield, prefix);
 }
 
+/**
+ * Compares any type
+ */
+void t_go_generator::generate_go_equals(std::ostream& out,
+                                           t_type* ori_type,

Review comment:
       nit: this indentation looks weird. usually you either align with `(`, or use fixed 2/4/6 spaces. this is neither.
   
   I don't believe clang-format is used to generate this indentation either, at least it's not enforced by travis. when I run `clang-format -i --style=file compiler/cpp/src/thrift/generate/t_go_generator.cc` it made a ton of changes in this file.

##########
File path: lib/go/test/EqualsTest.thrift
##########
@@ -0,0 +1,81 @@
+enum EnumFoo {
+  e1
+  e2
+}
+
+struct BasicEqualsFoo {
+  1: bool BoolFoo,
+  2: optional bool OptBoolFoo,
+  3: i8 I8Foo,
+  4: optional i8 OptI8Foo,
+  5: i16 I16Foo,
+  6: optional i16 OptI16Foo,
+  7: i32 I32Foo,
+  8: optional i32 OptI32Foo,
+  9: i64 I64Foo,
+  10: optional i64 OptI64Foo,
+  11: double DoubleFoo,
+  12: optional double OptDoubleFoo,
+  13: string StrFoo,
+  14: optional string OptStrFoo,
+  15: binary BinFoo,
+  16: optional binary OptBinFoo,
+  17: EnumFoo EnumFoo,
+  18: optional EnumFoo OptEnumFoo,
+}

Review comment:
       I would suggest also add a few typedef'd primitive types into this (and also into `ListEqualsFoo`, `SetEqualsFoo`, `MapEqualsFoo`).

##########
File path: lib/go/test/tests/equals_test.go
##########
@@ -0,0 +1,174 @@
+package tests
+
+import (
+	"equalstest"
+	"strconv"
+	"testing"
+)
+
+func TestEquals(t *testing.T) {
+	basicTgt, basicSrc := genBasicFoo(), genBasicFoo()
+	if !basicTgt.Equals(basicSrc) {
+		t.Fatal("BasicEqualsFoo.Equals() test failed")
+	}
+	basicSrc.EnumFoo = equalstest.EnumFoo_e2
+	if basicTgt.Equals(basicSrc) {
+		t.Fatal("BasicEqualsFoo.Equals() test failed")
+	}
+	structTgt, structSrc := genStructFoo(), genStructFoo()
+	if !structTgt.Equals(structSrc) {
+		t.Fatal("StructEqualsFoo.Equals() test failed")
+	}
+	structSrc.OptStructFoo.EnumFoo = equalstest.EnumFoo_e2
+	if structTgt.Equals(structSrc) {
+		t.Fatal("StructEqualsFoo.Equals() test failed")
+	}
+	listTgt, listSrc := genListFoo(), genListFoo()
+	if !listTgt.Equals(listSrc) {
+		t.Fatal("ListEqualsFoo.Equals() test failed")
+	}
+	listSrc.OptI64StringMapListFoo[0][1] = "0"
+	if listTgt.Equals(listSrc) {
+		t.Fatal("ListEqualsFoo.Equals() test failed")
+	}
+	setTgt, setSrc := genSetFoo(), genSetFoo()
+	if !setTgt.Equals(setSrc) {
+		t.Fatal("SetEqualsFoo.Equals() test failed")
+	}
+	setSrc.OptI64StringMapSetFoo[0][1] = "0"
+	if setTgt.Equals(setSrc) {
+		t.Fatal("SetEqualsFoo.Equals() test failed")
+	}
+	mapTgt, mapSrc := genMapFoo(), genMapFoo()
+	if !mapTgt.Equals(mapSrc) {
+		t.Fatal("MapEqualsFoo.Equals() test failed")
+	}
+	mapSrc.OptI64I64StringMapMapFoo[1][1] = "0"
+	if mapTgt.Equals(mapSrc) {
+		t.Fatal("MapEqualsFoo.Equals() test failed")
+	}
+}

Review comment:
       Another issue with the test is that all the optional fields are set, so if someone missed the nil check in the compiler (like in the first version of this PR) this test would not catch that error. I would suggest add a few more cases to each `gen*` call, while changing some of the optional fields to `nil` (for example, you should add a test to change one of the optional field to nil and make sure `Equals` return false, then change the other also to nil and make sure `Equals` return true).

##########
File path: lib/go/test/tests/equals_test.go
##########
@@ -0,0 +1,174 @@
+package tests
+
+import (
+	"equalstest"
+	"strconv"
+	"testing"
+)
+
+func TestEquals(t *testing.T) {
+	basicTgt, basicSrc := genBasicFoo(), genBasicFoo()
+	if !basicTgt.Equals(basicSrc) {
+		t.Fatal("BasicEqualsFoo.Equals() test failed")

Review comment:
       These test failures should be `t.Error` instead of `t.Fatal`. Fatal stops the test from running, so if someone made a mistake in the future and failed multiple cases inside this test, they will only see the first failure in the output.
   
   Fatal should only be used when the following tests are depending on this one (e.g. used for nil checks).




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org