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).
##########
File path: compiler/cpp/src/thrift/generate/t_go_generator.cc
##########
@@ -3463,6 +3543,109 @@ 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(ostream& out, t_type* ori_type, string
tgt, string src) {
+
+ t_type* ttype = get_true_type(ori_type);
+ // Do nothing for void types
+ if (ttype->is_void()) {
+ throw "compiler error: cannot generate equals for void type: " + tgt;
+ }
+
+ if (ttype->is_struct() || ttype->is_xception()) {
+ generate_go_equals_struct(out, ttype, tgt, src);
+ } else if (ttype->is_container()) {
+ generate_go_equals_container(out, ttype, tgt, src);
+ } else if (ttype->is_base_type() || ttype->is_enum()) {
+ out << indent() << "if ";
+ if (ttype->is_base_type()) {
+ t_base_type::t_base tbase = ((t_base_type*)ttype)->get_base();
+ switch (tbase) {
+ case t_base_type::TYPE_VOID:
+ throw "compiler error: cannot equals void: " + tgt;
+ break;
+
+ case t_base_type::TYPE_STRING:
+ if (ttype->is_binary()) {
+ out << "bytes.Compare(" << tgt << ", " << src << ") != 0";
+ } else {
+ out << tgt << " != " << src;
+ }
+ break;
+
+ case t_base_type::TYPE_BOOL:
+ case t_base_type::TYPE_I8:
+ case t_base_type::TYPE_I16:
+ case t_base_type::TYPE_I32:
+ case t_base_type::TYPE_I64:
+ case t_base_type::TYPE_DOUBLE:
+ out << tgt << " != " << src;
+ break;
+
+ default:
+ throw "compiler error: no Go name for base type " +
t_base_type::t_base_name(tbase);
+ }
+ } else if (ttype->is_enum()) {
+ out << tgt << " != " << src;
+ }
+
+ out << " { return false }" << endl;
+ } else {
+ throw "compiler error: Invalid type in generate_go_equals '" +
ttype->get_name() + "' for '"
+ + tgt + "'";
+ }
+}
+
+/**
+ * Compares the members of a struct
+ */
+void t_go_generator::generate_go_equals_struct(ostream& out,
+ t_type* ttype,
+ string tgt,
+ string src) {
+ (void)ttype;
Review comment:
what does this line do?
##########
File path: compiler/cpp/src/thrift/generate/t_go_generator.cc
##########
@@ -3463,6 +3543,109 @@ 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(ostream& out, t_type* ori_type, string
tgt, string src) {
+
+ t_type* ttype = get_true_type(ori_type);
+ // Do nothing for void types
+ if (ttype->is_void()) {
+ throw "compiler error: cannot generate equals for void type: " + tgt;
+ }
+
+ if (ttype->is_struct() || ttype->is_xception()) {
+ generate_go_equals_struct(out, ttype, tgt, src);
+ } else if (ttype->is_container()) {
+ generate_go_equals_container(out, ttype, tgt, src);
+ } else if (ttype->is_base_type() || ttype->is_enum()) {
+ out << indent() << "if ";
+ if (ttype->is_base_type()) {
+ t_base_type::t_base tbase = ((t_base_type*)ttype)->get_base();
+ switch (tbase) {
+ case t_base_type::TYPE_VOID:
+ throw "compiler error: cannot equals void: " + tgt;
+ break;
+
+ case t_base_type::TYPE_STRING:
+ if (ttype->is_binary()) {
+ out << "bytes.Compare(" << tgt << ", " << src << ") != 0";
+ } else {
+ out << tgt << " != " << src;
+ }
+ break;
+
+ case t_base_type::TYPE_BOOL:
+ case t_base_type::TYPE_I8:
+ case t_base_type::TYPE_I16:
+ case t_base_type::TYPE_I32:
+ case t_base_type::TYPE_I64:
+ case t_base_type::TYPE_DOUBLE:
+ out << tgt << " != " << src;
+ break;
+
+ default:
+ throw "compiler error: no Go name for base type " +
t_base_type::t_base_name(tbase);
+ }
+ } else if (ttype->is_enum()) {
+ out << tgt << " != " << src;
+ }
+
+ out << " { return false }" << endl;
+ } else {
+ throw "compiler error: Invalid type in generate_go_equals '" +
ttype->get_name() + "' for '"
+ + tgt + "'";
+ }
+}
+
+/**
+ * Compares the members of a struct
+ */
+void t_go_generator::generate_go_equals_struct(ostream& out,
+ t_type* ttype,
+ string tgt,
+ string src) {
+ (void)ttype;
+ out << indent() << "if !" << tgt << "." << equals_method_name_ << "(" << src
+ << ") { return false }" << endl;
+}
+
+/**
+ * Compares any container type
+ */
+void t_go_generator::generate_go_equals_container(ostream& out,
+ t_type* ttype,
+ string tgt,
+ string src) {
+ out << indent() << "if len(" << tgt << ") != len(" << src << ") { return
false }" << endl;
+ if (ttype->is_map()) {
+ t_map* tmap = (t_map*)ttype;
+ out << indent() << "for k, _tgt := range " << tgt << " {" << endl;
+ indent_up();
+ string element_source = tmp("_src");
+ out << indent() << element_source << " := " << src << "[k]" << endl;
+ generate_go_equals(out, tmap->get_val_type(), "_tgt", element_source);
+ indent_down();
+ indent(out) << "}" << endl;
+ } else if (ttype->is_list() || ttype->is_set()) {
Review comment:
should this just be `} else {`, or maybe add another `} else { throw
"unsupported container" }` at the end? currently if this is a container but
it's not a map/list/set (I'm not sure if we have other container types) this
function only checks for length and then does nothing.
##########
File path: lib/go/test/tests/equals_test.go
##########
@@ -0,0 +1,219 @@
+package tests
+
+import (
+ "equalstest"
+ "strconv"
+ "testing"
+)
+
+func TestEquals(t *testing.T) {
+ // test basic field
+ basicTgt, basicSrc := genBasicFoo(), genBasicFoo()
+ if !basicTgt.Equals(basicSrc) {
+ t.Error("BasicEqualsFoo.Equals() test failed")
+ }
+ basicSrc.EnumFoo = equalstest.EnumFoo_e2
+ if basicTgt.Equals(basicSrc) {
+ t.Error("BasicEqualsFoo.Equals() test failed")
+ }
+ basicSrc = genBasicFoo()
+ basicSrc.OptBoolFoo = nil
+ if basicTgt.Equals(basicSrc) || basicSrc.Equals(basicTgt) {
+ t.Error("BasicEqualsFoo.Equals() test failed")
+ }
+ if !(&equalstest.BasicEqualsFoo{}).Equals(&equalstest.BasicEqualsFoo{})
{
+ t.Error("BasicEqualsFoo.Equals() test failed")
+ }
+ // test struct field
+ structTgt, structSrc := genStructFoo(), genStructFoo()
+ if !structTgt.Equals(structSrc) {
+ t.Error("StructEqualsFoo.Equals() test failed")
+ }
+ structSrc.OptStructFoo.EnumFoo = equalstest.EnumFoo_e2
+ if structTgt.Equals(structSrc) {
+ t.Error("StructEqualsFoo.Equals() test failed")
+ }
+ structSrc = genStructFoo()
+ structSrc.OptStructFoo = nil
+ if structTgt.Equals(structSrc) || structSrc.Equals(structTgt) {
+ t.Error("StructEqualsFoo.Equals() test failed")
+ }
+ if
!(&equalstest.StructEqualsFoo{}).Equals(&equalstest.StructEqualsFoo{}) {
+ t.Error("StructEqualsFoo.Equals() test failed")
+ }
+ // test list field
+ listTgt, listSrc := genListFoo(), genListFoo()
+ if !listTgt.Equals(listSrc) {
+ t.Error("ListEqualsFoo.Equals() test failed")
+ }
+ listSrc.OptI64StringMapListFoo[0][1] = "0"
+ if listTgt.Equals(listSrc) {
+ t.Error("ListEqualsFoo.Equals() test failed")
+ }
+ listSrc = genListFoo()
+ listSrc.OptI64StringMapListFoo = nil
+ if listTgt.Equals(listSrc) || listSrc.Equals(listTgt) {
+ t.Error("ListEqualsFoo.Equals() test failed")
+ }
+ if !(&equalstest.ListEqualsFoo{}).Equals(&equalstest.ListEqualsFoo{}) {
+ t.Error("ListEqualsFoo.Equals() test failed")
+ }
+ // test set field
+ setTgt, setSrc := genSetFoo(), genSetFoo()
+ if !setTgt.Equals(setSrc) {
+ t.Error("SetEqualsFoo.Equals() test failed")
+ }
+ setSrc.OptI64StringMapSetFoo[0][1] = "0"
+ if setTgt.Equals(setSrc) {
+ t.Error("SetEqualsFoo.Equals() test failed")
+ }
+ setSrc = genSetFoo()
+ setSrc.OptI64StringMapSetFoo = nil
+ if setTgt.Equals(setSrc) || setSrc.Equals(setTgt) {
+ t.Error("SetEqualsFoo.Equals() test failed")
+ }
+ if !(&equalstest.SetEqualsFoo{}).Equals(&equalstest.SetEqualsFoo{}) {
+ t.Error("SetEqualsFoo.Equals() test failed")
+ }
+ // test map field
+ mapTgt, mapSrc := genMapFoo(), genMapFoo()
+ if !mapTgt.Equals(mapSrc) {
+ t.Error("MapEqualsFoo.Equals() test failed")
+ }
+ mapSrc.OptI64I64StringMapMapFoo[1][1] = "0"
+ if mapTgt.Equals(mapSrc) {
+ t.Error("MapEqualsFoo.Equals() test failed")
+ }
+ mapSrc = genMapFoo()
+ mapSrc.OptI64I64StringMapMapFoo = nil
+ if mapTgt.Equals(mapSrc) || mapSrc.Equals(mapTgt) {
+ t.Error("MapEqualsFoo.Equals() test failed")
+ }
+ if !(&equalstest.MapEqualsFoo{}).Equals(&equalstest.MapEqualsFoo{}) {
+ t.Error("MapEqualsFoo.Equals() test failed")
+ }
+}
+
+func genBasicFoo() *equalstest.BasicEqualsFoo {
+ return &equalstest.BasicEqualsFoo{
+ BoolFoo: true,
+ OptBoolFoo: &(&struct{ x bool }{true}).x,
+ I8Foo: 1,
+ OptI8Foo: &(&struct{ x int8 }{1}).x,
+ I16Foo: 2,
+ OptI16Foo: &(&struct{ x int16 }{2}).x,
+ I32Foo: 3,
+ OptI32Foo: &(&struct{ x int32 }{3}).x,
+ I64Foo: 4,
+ OptI64Foo: &(&struct{ x int64 }{4}).x,
+ DoubleFoo: 5,
+ OptDoubleFoo: &(&struct{ x float64 }{5}).x,
+ StrFoo: "6",
+ OptStrFoo: &(&struct{ x string }{"6"}).x,
+ BinFoo: []byte("7"),
+ OptBinFoo: []byte("7"),
+ EnumFoo: equalstest.EnumFoo_e1,
+ OptEnumFoo: &(&struct{ x equalstest.EnumFoo
}{equalstest.EnumFoo_e1}).x,
+ }
Review comment:
the newly added typedef'd fields are not added here (and other `gen*`
functions)
##########
File path: test/go/src/bin/testequals/main.go
##########
@@ -0,0 +1,179 @@
+package main
Review comment:
hmm why do we need to add this directory/file?
----------------------------------------------------------------
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:
[email protected]
