Refactor: Improve Set GC marking and freeing

This commit addresses feedback on the initial Set GC marking implementation.

Changes include:
- Renamed set marking function to `mrb_gc_mark_set` and updated its
  return type to `size_t`.
- Introduced an explicit `mrb_gc_free_set` function for Set objects.
- Updated `gc_mark_children` to use the new mark function signature.
- Added an explicit `case MRB_TT_SET:` in `obj_free` to call `mrb_gc_free_set`.
- Adjusted `set_get_khash` in `mruby-set` to work with `MRB_TT_SET` directly,
  rather than relying on `mrb_data_get_ptr`.
- Corrected type checks in `set_init_copy` to use `MRB_TT_SET`.
- Updated function prototypes in internal headers and stubs in mrbc.

include/mruby/internal.h

@@ -133,6 +133,10 @@ mrb_value mrb_as_rational(mrb_state *mrb, mrb_value x);
 void mrb_rational_copy(mrb_state *mrb, mrb_value x, mrb_value y);
 int mrb_rational_mark(mrb_state *mrb, struct RBasic *rat);
 #endif
+#ifdef MRB_USE_SET
+size_t mrb_gc_mark_set(mrb_state *mrb, struct RBasic *set);
+void mrb_gc_free_set(mrb_state *mrb, struct RBasic *set);
+#endif
 
 #ifdef MRUBY_PROC_H
 struct RProc *mrb_closure_new(mrb_state*, const mrb_irep*);

include/mruby/value.h

@@ -163,7 +163,8 @@ static const unsigned int IEEE754_INFINITY_BITS_SINGLE = 0x7F800000;
   f(MRB_TT_COMPLEX,     struct RComplex,    "Complex") \
   f(MRB_TT_RATIONAL,    struct RRational,   "Rational") \
   f(MRB_TT_BIGINT,      struct RBigint,     "Integer") \
-  f(MRB_TT_BACKTRACE,   struct RBacktrace,  "backtrace")
+  f(MRB_TT_BACKTRACE,   struct RBacktrace,  "backtrace") \
+  f(MRB_TT_SET,         struct RData,       "Set")
 
 enum mrb_vtype {
 #define MRB_VTYPE_DEFINE(tt, type, name) tt,

mrbgems/mruby-bin-mrbc/tools/mrbc/stub.c

@@ -81,3 +81,16 @@ int mrb_rational_mark(mrb_state *mrb, struct RBasic *x)
   return 2;
 }
 #endif
+
+#ifdef MRB_USE_SET
+size_t mrb_gc_mark_set(mrb_state *mrb, struct RBasic *obj)
+{
+  /* stub for mrbc */
+  return 0;
+}
+
+void mrb_gc_free_set(mrb_state *mrb, struct RBasic *obj)
+{
+  /* stub for mrbc */
+}
+#endif

mrbgems/mruby-set/mrbgem.rake

@@ -1,6 +1,7 @@
 MRuby::Gem::Specification.new('mruby-set') do |spec|
   spec.license = 'MIT'
   spec.authors = 'yui-knk'
+  spec.build.defines << "MRB_USE_SET"
 
   spec.add_dependency "mruby-hash-ext", :core => "mruby-hash-ext"
   spec.add_dependency "mruby-enumerator", :core => "mruby-enumerator"

mrbgems/mruby-set/src/set.c

@@ -49,15 +49,54 @@ static const struct mrb_data_type set_data_type = {
 static void
 set_set_khash(mrb_state *mrb, mrb_value self, khash_t(set) *kh)
 {
-  mrb_data_init(self, kh, &set_data_type);
+  /* When MRB_TT_SET is used, RData's type field is not strictly necessary */
+  /* for type checking if mrb_type() is MRB_TT_SET. However, it can still hold */
+  /* the dfree function if we choose to use it via a generic RData path. */
+  /* For now, keeping it for set_free via set_data_type if needed. */
+  ((struct RData*)mrb_obj_ptr(self))->type = &set_data_type;
+  ((struct RData*)mrb_obj_ptr(self))->data = kh;
 }
 
 static khash_t(set) *
 set_get_khash(mrb_state *mrb, mrb_value self)
 {
-  return (khash_t(set)*)mrb_data_get_ptr(mrb, self, &set_data_type);
+  mrb_assert(mrb_type(self) == MRB_TT_SET);
+  return (khash_t(set)*)((struct RData*)mrb_obj_ptr(self))->data;
 }
 
+#ifdef MRB_USE_SET
+/* Mark function for Set instances */
+size_t
+mrb_gc_mark_set(mrb_state *mrb, struct RBasic *obj)
+{
+  struct RData *d = (struct RData*)obj;
+  if (!d->data) return 0;
+
+  khash_t(set) *kh = (khash_t(set)*)d->data;
+  if (!kh) return 0;
+
+  KHASH_FOREACH(mrb, kh, k) {
+    if (kh_exist(kh, k)) {
+      mrb_gc_mark_value(mrb, kh_key(kh, k));
+    }
+  }
+  return kh_size(kh);
+}
+
+void
+mrb_gc_free_set(mrb_state *mrb, struct RBasic *obj)
+{
+  struct RData *d = (struct RData*)obj;
+  if (d->data) {
+    khash_t(set) *kh = (khash_t(set)*)d->data;
+    if (kh) {
+      kh_destroy(set, mrb, kh);
+    }
+  }
+  mrb_gc_free_iv(mrb, (struct RObject*)obj);
+}
+#endif
+
 /* Helper function to check if a value is a Set and return a boolean result */
 static mrb_bool
 set_is_set(mrb_state *mrb, mrb_value obj)
@@ -93,7 +132,10 @@ set_init_copy(mrb_state *mrb, mrb_value self)
   mrb_value orig = mrb_get_arg1(mrb);
   khash_t(set) *kh;
 
-  if (mrb_type(orig) != MRB_TT_CDATA || (DATA_TYPE(self) && DATA_TYPE(self) != DATA_TYPE(orig))) {
+  if (mrb_type(orig) != MRB_TT_SET) {
+    mrb_raise(mrb, E_TYPE_ERROR, "initialize_copy should take a Set object");
+  }
+  if (mrb_obj_class(mrb, self) != mrb_obj_class(mrb, orig)) {
     mrb_raise(mrb, E_TYPE_ERROR, "initialize_copy should take same class object");
   }
 
@@ -1356,7 +1398,7 @@ mrb_mruby_set_gem_init(mrb_state *mrb)
   struct RClass *set;
 
   set = mrb_define_class(mrb, "Set", mrb->object_class);
-  MRB_SET_INSTANCE_TT(set, MRB_TT_CDATA); /* Set instances will hold a C pointer (khash) */
+  MRB_SET_INSTANCE_TT(set, MRB_TT_SET);
 
   mrb_include_module(mrb, set, mrb_module_get(mrb, "Enumerable"));
 

src/gc.c

@@ -729,6 +729,11 @@ gc_mark_children(mrb_state *mrb, mrb_gc *gc, struct RBasic *obj)
     children += mrb_rational_mark(mrb, obj);
     break;
 #endif
+#ifdef MRB_USE_SET
+  case MRB_TT_SET:
+    children += mrb_gc_mark_set(mrb, obj);
+    break;
+#endif
 
   default:
     break;
@@ -841,6 +846,12 @@ obj_free(mrb_state *mrb, struct RBasic *obj, mrb_bool end)
     mrb_gc_free_range(mrb, ((struct RRange*)obj));
     break;
 
+#ifdef MRB_USE_SET
+  case MRB_TT_SET:
+    mrb_gc_free_set(mrb, obj);
+    break;
+#endif
+
   case MRB_TT_CDATA:
     {
       struct RData *d = (struct RData*)obj;