Major rework to the union support

[jwillemsen]

Leverage std::variant as type-safe union which reduces the generated code for IDL unions significantly.

Summary by CodeRabbit

• Improvements

  • Accessors and discriminator getters marked nodiscard.
  • Several special member operations (constructors, assignments, destructor) defaulted for clearer semantics.

• Refactor

  • Union storage and initialization moved to an index-based, variant-backed approach with simplified swap and init behavior.
  • Generated headers now include variant support.

• Tests

  • Added a typedef and union test plus an extra runtime check validating default-member behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

Replace manual union storage/management with std::variant and index-based in-place initialization; switch special-member declarations to = default; rename value_initializer → default_value; add <variant> pre-include; add typedef and union in IDL and extend union tests.

Changes

Cohort / File(s)	Summary
Union header template ridlbe/c++11/templates/cli/hdr/union_post.erb	Switch special members to = default; remove union-based storage and helper declarations; introduce using u_type_ = std::variant<...> and u_ with in-place index initialization; add [[nodiscard]] to accessors.
Union implementation template ridlbe/c++11/templates/cli/inl/union_inl.erb	Refactor per-member union logic to uniform index-based handling: remove _clear/_swap_u/_move_u, use u_.emplace<index>(...), std::get<index>(u_), and std::swap(u_); simplify constructors, assignment, swap, and default init.
Codegen visitor & writer ridlbe/c++11/visitors/union.rb, ridlbe/c++11/writers/stubheader.rb	Rename value_initializer → default_value and return typedef-resolved defaults as strings; add pre-include for <variant> when emitting union stub headers.
Tests and IDL tests/union/test.idl, tests/union/client.cpp, tests/union/checks.h	Add typedef long mylong; and union DefaultDataTypeDef using typedef default; extend client.cpp with additional mutation/verification of z_string(); minor formatting cleanup in checks.h.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 I swapped my burrow for a variant coat,
I defaulted constructors and kept my note,
Nodiscard whiskers peek at every field,
Typedef crumbs and tests revealed,
Hooray — a tidy union for my tote! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: a comprehensive rework of union support from union-based storage to std::variant-based storage throughout the C++11 code generator and tests.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@tests/union/test.idl`:
- Around line 187-193: Add runtime test coverage for the DefaultDataTypeDef
union (and its typedef mylong) by mirroring the existing tests for DefaultData:
declare matching interface methods in the IDL (same signatures used for
DefaultData) and implement runtime checks in client.cpp (e.g., a new test
function similar to test_data_default_union()) that exercises the case 1, case 2
and default mylong variant, performs round-trip calls and asserts correct
discriminator/value handling to ensure the typedef chain is honored by codegen
and execution.

🧹 Nitpick comments (1)

ridlbe/c++11/visitors/union.rb (1)

194-209: Consider extracting shared logic with value_initializer.

The default_value method closely mirrors value_initializer (lines 177-192) — both traverse the typedef chain looking for @default annotations. The only difference is the output format (with vs. without braces).

This duplication could be reduced by extracting the shared traversal logic into a private helper.

♻️ Optional refactor to reduce duplication

+    private
+
+    def find_default_annotation_value
+      unless node.annotations[:default].first.nil?
+        return node.annotations[:default].first.fields[:value]
+      end
+      res_idl_type = _idltype
+      while res_idl_type.is_a?(IDL::Type::ScopedName)
+        unless res_idl_type.node.annotations[:default].first.nil?
+          return res_idl_type.node.annotations[:default].first.fields[:value]
+        end
+        res_idl_type = res_idl_type.node.idltype
+      end
+      nil
+    end
+
+    public
+
     def value_initializer
-      unless node.annotations[:default].first.nil?
-        "{#{node.annotations[:default].first.fields[:value]}}"
-      else
-        res_idl_type = _idltype
-        while res_idl_type.is_a?(IDL::Type::ScopedName)
-          unless res_idl_type.node.annotations[:default].first.nil?
-            return "{#{res_idl_type.node.annotations[:default].first.fields[:value]}}"
-          end
-          res_idl_type = res_idl_type.node.idltype
-        end
-        _resolved_idltype.value_initializer
-      end
+      val = find_default_annotation_value
+      val ? "{#{val}}" : _resolved_idltype.value_initializer
     end

     def default_value
-      unless node.annotations[:default].first.nil?
-        "#{node.annotations[:default].first.fields[:value]}"
-      else
-        res_idl_type = _idltype
-        while res_idl_type.is_a?(IDL::Type::ScopedName)
-          unless res_idl_type.node.annotations[:default].first.nil?
-            return "#{res_idl_type.node.annotations[:default].first.fields[:value]}"
-          end
-          res_idl_type = res_idl_type.node.idltype
-        end
-        _resolved_idltype.default_value
-      end
+      val = find_default_annotation_value
+      val ? val.to_s : _resolved_idltype.default_value
     end

[jwillemsen]

See RemedyIT/idl2cpp11#81 how this PR heavily reduces the generated code for unions

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@ridlbe/c`++11/templates/cli/inl/union_inl.erb:
- Around line 150-156: The move setter unconditionally assigns this->disc_ =
_x11_disc even when _x11_disc isn’t a parameter (only present when
_m.has_multiple_discriminators?); fix by using the correct discriminator value:
if _m.has_multiple_discriminators? keep using _x11_disc, otherwise assign the
concrete label (e.g., the first nondefault label used previously) — compute that
label as _lbl in this scope or hoist its computation so the move setter can set
this->disc_ correctly before emplacing into this->u_.emplace<<%= _index
%>>(std::move(_x11_<%= _m.cxxname %>)).

🧹 Nitpick comments (2)

ridlbe/c++11/templates/cli/hdr/union_post.erb (2)

26-26: Consider adding [[nodiscard]] to the discriminator getter.

The AI summary indicates _d() was updated to use [[nodiscard]], but the code shows it without this attribute. Based on learnings, TAOX11 requires a C++17 capable compiler, so [[nodiscard]] is safe to use in generated code and would be beneficial here to warn callers who discard the discriminator value.

,

Suggested change

-  inline <%= switch_in_cxxtype %> _d () const  { return this->disc_; }
+  [[nodiscard]] inline <%= switch_in_cxxtype %> _d () const  { return this->disc_; }

---

44-49: Consider adding [[nodiscard]] to member accessors.

Similarly, the AI summary mentions [[nodiscard]] on member accessors, but these declarations lack the attribute. Since discarding the return value of these getters is almost certainly a logic error, [[nodiscard]] would help catch such mistakes at compile time.

,

Suggested change

-  inline <%= _m.cxx_in_type %> <%= _m.cxxname %> () const;
+  [[nodiscard]] inline <%= _m.cxx_in_type %> <%= _m.cxxname %> () const;
   /// Get the value of the union, if the discriminator doesn't match a
   /// BAD_PARAM exception is thrown
-  inline <%= _m.cxx_out_type %> <%= _m.cxxname %> ();
+  [[nodiscard]] inline <%= _m.cxx_out_type %> <%= _m.cxxname %> ();