Fix issue #457

This fixes the issue of comparison operators not returning true for unions with default cases not associated with a type for said default case Also added unittests for union comparison operators Signed-off-by: Martijn Reicher <[email protected]>
eclipse-cyclonedds · Nov 7, 2023 · 1e8b263 · 1e8b263
1 parent 0ddd39b
commit 1e8b263
Show file tree

Hide file tree

Showing 4 changed files with 102 additions and 6 deletions.
diff --git a/src/ddscxx/tests/Regression.cpp b/src/ddscxx/tests/Regression.cpp
@@ -404,4 +404,63 @@ TEST_F(Regression, unaligned_access)
   ASSERT_EQ(s.ll(), int64_t(0x08090A0B0C0D0E0F));  //size 4 reads should be done at 4 byte offsets in stream
 }
 
+TEST_F(Regression, union_comparisons)
+{
+  regression_models::union_without_default u_1, u_2, u_3, u_4;
+  u_1.s("abcdef", regression_enum::case_1);
+  u_2.s("fedcba", regression_enum::case_1);
+  u_3.s("abcdef", regression_enum::case_2);
+  u_4._d(regression_enum::case_3);
+
+  regression_models::union_with_default w_1, w_2, w_3, w_4, w_5;
+  w_1.s("abcdef", regression_enum::case_1);
+  w_2.s("fedcba", regression_enum::case_1);
+  w_3.s("abcdef", regression_enum::case_2);
+  w_4.i(123);
+  w_5.i(456);
+
+  EXPECT_EQ(u_1, u_1);
+  EXPECT_NE(u_1, u_2);
+  EXPECT_NE(u_1, u_3);
+  EXPECT_NE(u_1, u_4);
+  EXPECT_NE(u_2, u_1);
+  EXPECT_EQ(u_2, u_2);
+  EXPECT_NE(u_2, u_3);
+  EXPECT_NE(u_2, u_4);
+  EXPECT_NE(u_3, u_1);
+  EXPECT_NE(u_3, u_2);
+  EXPECT_EQ(u_3, u_3);
+  EXPECT_NE(u_3, u_4);
+  EXPECT_NE(u_4, u_1);
+  EXPECT_NE(u_4, u_2);
+  EXPECT_NE(u_4, u_3);
+  EXPECT_EQ(u_4, u_4);
+
+  EXPECT_EQ(w_1, w_1);
+  EXPECT_NE(w_1, w_2);
+  EXPECT_NE(w_1, w_3);
+  EXPECT_NE(w_1, w_4);
+  EXPECT_NE(w_1, w_5);
+  EXPECT_NE(w_2, w_1);
+  EXPECT_EQ(w_2, w_2);
+  EXPECT_NE(w_2, w_3);
+  EXPECT_NE(w_2, w_4);
+  EXPECT_NE(w_2, w_5);
+  EXPECT_NE(w_3, w_1);
+  EXPECT_NE(w_3, w_2);
+  EXPECT_EQ(w_3, w_3);
+  EXPECT_NE(w_3, w_4);
+  EXPECT_NE(w_3, w_5);
+  EXPECT_NE(w_4, w_1);
+  EXPECT_NE(w_4, w_2);
+  EXPECT_NE(w_4, w_3);
+  EXPECT_EQ(w_4, w_4);
+  EXPECT_NE(w_4, w_5);
+  EXPECT_NE(w_5, w_1);
+  EXPECT_NE(w_5, w_2);
+  EXPECT_NE(w_5, w_3);
+  EXPECT_NE(w_5, w_4);
+  EXPECT_EQ(w_5, w_5);
+}
+
 DDSRT_WARNING_GNUC_ON(maybe-uninitialized)
diff --git a/src/ddscxx/tests/data/RegressionModels.idl b/src/ddscxx/tests/data/RegressionModels.idl
@@ -188,4 +188,24 @@ struct s_unaligned_access {
   long long ll;
 };
 
+enum regression_enum {
+    case_1,
+    case_2,
+    case_3
+};
+
+union union_without_default switch (regression_enum) {
+    case case_1:
+    case case_2:
+        string s;
+};
+
+union union_with_default switch (regression_enum) {
+    case case_1:
+    case case_2:
+        string s;
+    default:
+        long i;
+};
+
 };
diff --git a/src/idlcxx/src/streamers.c b/src/idlcxx/src/streamers.c
@@ -964,11 +964,12 @@ process_case(
     if (idl_next(_case)) {
       return IDL_RETCODE_OK;
     } else {
-      //if last entry, and no default case was present for this union
-      const idl_case_label_t *def = _union->default_case;
-      if (idl_is_union(def->node.parent) &&
-          multi_putf(streams, READ, "    default:\n      instance._d(d);\n"))
-        return IDL_RETCODE_NO_MEMORY;
+      //if last entry, and the default case is an implicit default
+      if (_union == idl_parent(_union->default_case)) {
+        if (multi_putf(streams, READ, "    default:\n      instance._d(d);\n") ||
+            multi_putf(streams, WRITE | MOVE, "    default:\n      break;\n"))
+          return IDL_RETCODE_NO_MEMORY;
+      }
     }
 
     if (multi_putf(streams, NOMAX, "  }\n"))

diff --git a/src/idlcxx/src/types.c b/src/idlcxx/src/types.c
@@ -926,6 +926,8 @@ emit_union(
   memset(&visitor, 0, sizeof(visitor));
 
   _union = node;
+  /* check whether default label is associated with a union case */
+  const bool gen_default = (idl_parent(_union->default_case) == _union);
   type_spec = _union->switch_type_spec->type_spec;
 
   name = get_cpp11_name(_union);
@@ -1016,6 +1018,13 @@ emit_union(
   if ((ret = idl_visit(pstate, _union->cases, &visitor, user_data)))
     return ret;
 
+  if (gen_default) {
+    fmt = "      default:\n"
+          "        break;\n";
+    if (idl_fprintf(gen->header.handle, fmt, type) < 0)
+      return IDL_RETCODE_NO_MEMORY;
+  }
+
   fmt = "    }\n"
         "    return _default_discriminator;\n"
         "  }\n\n"
@@ -1073,8 +1082,15 @@ emit_union(
   if ((ret = idl_visit(pstate, _union->cases, &visitor, user_data)))
     return ret;
 
+  if (gen_default) {
+    fmt =  "      default:\n"
+           "        return true;\n";
+    if (idl_fprintf(gen->header.handle, fmt, name) < 0)
+      return IDL_RETCODE_NO_MEMORY;
+  }
+
   fmt = "    }\n"
-        "    return false;\n"
+        "    return true;\n"
         "  }\n\n"
         "  bool operator!=(const %s& _other) const\n"
         "  {\n"