Encapsulate RCLASS_ATTACHED_OBJECT

Right now the attached object is stored as an instance variable and all the call sites that either get or set it have to know how it's stored. It's preferable to hide this implementation detail behind accessors so that it is easier to change how it's stored.
2023-02-15 10:42:52 +01:00 · 2023-02-15 10:42:52 +01:00 · 7413079dae
--- a/class.c
+++ b/class.c
@ -30,8 +30,6 @@
 #include "ruby/st.h"
 #include "vm_core.h"

-#define id_attached id__attached__
-
 #define METACLASS_OF(k) RBASIC(k)->klass
 #define SET_METACLASS_OF(k, cls) RBASIC_SET_CLASS(k, cls)

@ -615,7 +613,7 @@ rb_singleton_class_clone_and_attach(VALUE obj, VALUE attach)
    // attached to an object other than `obj`. In which case `obj` does not have
    // a material singleton class attached yet and there is no singleton class
    // to clone.
-    if (!(FL_TEST(klass, FL_SINGLETON) && rb_attr_get(klass, id_attached) == obj)) {
+    if (!(FL_TEST(klass, FL_SINGLETON) && RCLASS_ATTACHED_OBJECT(klass) == obj)) {
        // nothing to clone
        return klass;
    }
@ -668,7 +666,7 @@ void
 rb_singleton_class_attached(VALUE klass, VALUE obj)
 {
    if (FL_TEST(klass, FL_SINGLETON)) {
-        rb_class_ivar_set(klass, id_attached, obj);
+        RCLASS_SET_ATTACHED_OBJECT(klass, obj);
    }
 }

@ -682,13 +680,13 @@ rb_singleton_class_attached(VALUE klass, VALUE obj)
 static int
 rb_singleton_class_has_metaclass_p(VALUE sklass)
 {
-    return rb_attr_get(METACLASS_OF(sklass), id_attached) == sklass;
+    return RCLASS_ATTACHED_OBJECT(METACLASS_OF(sklass)) == sklass;
 }

 int
 rb_singleton_class_internal_p(VALUE sklass)
 {
-    return (RB_TYPE_P(rb_attr_get(sklass, id_attached), T_CLASS) &&
+    return (RB_TYPE_P(RCLASS_ATTACHED_OBJECT(sklass), T_CLASS) &&
            !rb_singleton_class_has_metaclass_p(sklass));
 }

@ -1677,7 +1675,7 @@ rb_class_attached_object(VALUE klass)
        rb_raise(rb_eTypeError, "`%"PRIsVALUE"' is not a singleton class", klass);
    }

-    return rb_attr_get(klass, id_attached);
+    return RCLASS_ATTACHED_OBJECT(klass);
 }

 static void
@ -2206,7 +2204,7 @@ singleton_class_of(VALUE obj)

    klass = METACLASS_OF(obj);
    if (!(FL_TEST(klass, FL_SINGLETON) &&
-          rb_attr_get(klass, id_attached) == obj)) {
+          RCLASS_ATTACHED_OBJECT(klass) == obj)) {
        klass = rb_make_metaclass(obj, klass);
    }

@ -2245,7 +2243,7 @@ rb_singleton_class_get(VALUE obj)
    }
    klass = METACLASS_OF(obj);
    if (!FL_TEST(klass, FL_SINGLETON)) return Qnil;
-    if (rb_attr_get(klass, id_attached) != obj) return Qnil;
+    if (RCLASS_ATTACHED_OBJECT(klass) != obj) return Qnil;
    return klass;
 }

--- a/common.mk
+++ b/common.mk
@ -3464,7 +3464,6 @@ cont.$(OBJEXT): {$(VPATH)}id.h
 cont.$(OBJEXT): {$(VPATH)}id_table.h
 cont.$(OBJEXT): {$(VPATH)}intern.h
 cont.$(OBJEXT): {$(VPATH)}internal.h
-cont.$(OBJEXT): {$(VPATH)}internal/gc.h
 cont.$(OBJEXT): {$(VPATH)}internal/abi.h
 cont.$(OBJEXT): {$(VPATH)}internal/anyargs.h
 cont.$(OBJEXT): {$(VPATH)}internal/arithmetic.h
@ -3676,7 +3675,6 @@ debug.$(OBJEXT): {$(VPATH)}id.h
 debug.$(OBJEXT): {$(VPATH)}id_table.h
 debug.$(OBJEXT): {$(VPATH)}intern.h
 debug.$(OBJEXT): {$(VPATH)}internal.h
-debug.$(OBJEXT): {$(VPATH)}internal/gc.h
 debug.$(OBJEXT): {$(VPATH)}internal/abi.h
 debug.$(OBJEXT): {$(VPATH)}internal/anyargs.h
 debug.$(OBJEXT): {$(VPATH)}internal/arithmetic.h
@ -5935,6 +5933,7 @@ enumerator.$(OBJEXT): $(top_srcdir)/internal/serial.h
 enumerator.$(OBJEXT): $(top_srcdir)/internal/static_assert.h
 enumerator.$(OBJEXT): $(top_srcdir)/internal/string.h
 enumerator.$(OBJEXT): $(top_srcdir)/internal/struct.h
+enumerator.$(OBJEXT): $(top_srcdir)/internal/variable.h
 enumerator.$(OBJEXT): $(top_srcdir)/internal/vm.h
 enumerator.$(OBJEXT): $(top_srcdir)/internal/warnings.h
 enumerator.$(OBJEXT): {$(VPATH)}assert.h
@ -5948,6 +5947,7 @@ enumerator.$(OBJEXT): {$(VPATH)}backward/2/long_long.h
 enumerator.$(OBJEXT): {$(VPATH)}backward/2/stdalign.h
 enumerator.$(OBJEXT): {$(VPATH)}backward/2/stdarg.h
 enumerator.$(OBJEXT): {$(VPATH)}config.h
+enumerator.$(OBJEXT): {$(VPATH)}constant.h
 enumerator.$(OBJEXT): {$(VPATH)}defines.h
 enumerator.$(OBJEXT): {$(VPATH)}encoding.h
 enumerator.$(OBJEXT): {$(VPATH)}enumerator.c
@ -6376,7 +6376,6 @@ eval.$(OBJEXT): {$(VPATH)}id.h
 eval.$(OBJEXT): {$(VPATH)}id_table.h
 eval.$(OBJEXT): {$(VPATH)}intern.h
 eval.$(OBJEXT): {$(VPATH)}internal.h
-eval.$(OBJEXT): {$(VPATH)}internal/gc.h
 eval.$(OBJEXT): {$(VPATH)}internal/abi.h
 eval.$(OBJEXT): {$(VPATH)}internal/anyargs.h
 eval.$(OBJEXT): {$(VPATH)}internal/arithmetic.h
@ -8967,6 +8966,7 @@ marshal.$(OBJEXT): {$(VPATH)}constant.h
 marshal.$(OBJEXT): {$(VPATH)}defines.h
 marshal.$(OBJEXT): {$(VPATH)}encindex.h
 marshal.$(OBJEXT): {$(VPATH)}encoding.h
+marshal.$(OBJEXT): {$(VPATH)}id.h
 marshal.$(OBJEXT): {$(VPATH)}id_table.h
 marshal.$(OBJEXT): {$(VPATH)}intern.h
 marshal.$(OBJEXT): {$(VPATH)}internal.h
@ -17369,6 +17369,7 @@ vm_backtrace.$(OBJEXT): $(CCAN_DIR)/str/str.h
 vm_backtrace.$(OBJEXT): $(hdrdir)/ruby/ruby.h
 vm_backtrace.$(OBJEXT): $(top_srcdir)/internal/array.h
 vm_backtrace.$(OBJEXT): $(top_srcdir)/internal/basic_operators.h
+vm_backtrace.$(OBJEXT): $(top_srcdir)/internal/class.h
 vm_backtrace.$(OBJEXT): $(top_srcdir)/internal/compilers.h
 vm_backtrace.$(OBJEXT): $(top_srcdir)/internal/error.h
 vm_backtrace.$(OBJEXT): $(top_srcdir)/internal/gc.h
@ -17959,6 +17960,7 @@ vm_trace.$(OBJEXT): $(hdrdir)/ruby.h
 vm_trace.$(OBJEXT): $(hdrdir)/ruby/ruby.h
 vm_trace.$(OBJEXT): $(top_srcdir)/internal/array.h
 vm_trace.$(OBJEXT): $(top_srcdir)/internal/basic_operators.h
+vm_trace.$(OBJEXT): $(top_srcdir)/internal/class.h
 vm_trace.$(OBJEXT): $(top_srcdir)/internal/compilers.h
 vm_trace.$(OBJEXT): $(top_srcdir)/internal/gc.h
 vm_trace.$(OBJEXT): $(top_srcdir)/internal/hash.h
--- a/eval.c
+++ b/eval.c
@ -441,7 +441,7 @@ rb_class_modify_check(VALUE klass)

        if (FL_TEST(klass, FL_SINGLETON)) {
            desc = "object";
-            klass = rb_ivar_get(klass, id__attached__);
+            klass = RCLASS_ATTACHED_OBJECT(klass);
            if (!SPECIAL_CONST_P(klass)) {
                switch (BUILTIN_TYPE(klass)) {
                  case T_MODULE:
--- a/ext/objspace/depend
+++ b/ext/objspace/depend
@ -342,6 +342,7 @@ objspace.o: $(hdrdir)/ruby/regex.h
 objspace.o: $(hdrdir)/ruby/ruby.h
 objspace.o: $(hdrdir)/ruby/st.h
 objspace.o: $(hdrdir)/ruby/subst.h
+objspace.o: $(top_srcdir)/constant.h
 objspace.o: $(top_srcdir)/id_table.h
 objspace.o: $(top_srcdir)/internal.h
 objspace.o: $(top_srcdir)/internal/array.h
@ -353,6 +354,7 @@ objspace.o: $(top_srcdir)/internal/imemo.h
 objspace.o: $(top_srcdir)/internal/sanitizers.h
 objspace.o: $(top_srcdir)/internal/serial.h
 objspace.o: $(top_srcdir)/internal/static_assert.h
+objspace.o: $(top_srcdir)/internal/variable.h
 objspace.o: $(top_srcdir)/internal/warnings.h
 objspace.o: $(top_srcdir)/node.h
 objspace.o: $(top_srcdir)/shape.h
--- a/internal/class.h
+++ b/internal/class.h
@ -8,9 +8,11 @@
 *             file COPYING are met.  Consult the file for details.
 * @brief      Internal header for Class.
 */
+#include "id.h"
 #include "id_table.h"           /* for struct rb_id_table */
 #include "internal/serial.h"    /* for rb_serial_t */
 #include "internal/static_assert.h"
+#include "internal/variable.h"  /* for rb_class_ivar_set */
 #include "ruby/internal/stdbool.h"     /* for bool */
 #include "ruby/intern.h"        /* for rb_alloc_func_t */
 #include "ruby/ruby.h"          /* for struct RBasic */
@ -99,6 +101,7 @@ STATIC_ASSERT(sizeof_rb_classext_t, sizeof(struct RClass) + sizeof(rb_classext_t
 #define RCLASS_SUBCLASSES(c) (RCLASS_EXT(c)->subclasses)
 #define RCLASS_SUPERCLASS_DEPTH(c) (RCLASS_EXT(c)->superclass_depth)
 #define RCLASS_SUPERCLASSES(c) (RCLASS_EXT(c)->superclasses)
+#define RCLASS_ATTACHED_OBJECT(c) (rb_attr_get(c, id__attached__))

 #define RICLASS_IS_ORIGIN FL_USER0
 #define RCLASS_CLONED     FL_USER1
@ -197,4 +200,14 @@ RCLASS_SET_CLASSPATH(VALUE klass, VALUE classpath, bool permanent)
    RCLASS_EXT(klass)->permanent_classpath = permanent;
 }

+static inline VALUE
+RCLASS_SET_ATTACHED_OBJECT(VALUE klass, VALUE attached_object)
+{
+    assert(BUILTIN_TYPE(klass) == T_CLASS);
+    assert(FL_TEST(klass, FL_SINGLETON));
+
+    rb_class_ivar_set(klass, id__attached__, attached_object);
+    return attached_object;
+}
+
 #endif /* INTERNAL_CLASS_H */
--- a/object.c
+++ b/object.c
@ -1587,7 +1587,7 @@ rb_mod_to_s(VALUE klass)

    if (FL_TEST(klass, FL_SINGLETON)) {
        VALUE s = rb_usascii_str_new2("#<Class:");
-        VALUE v = rb_ivar_get(klass, id__attached__);
+        VALUE v = RCLASS_ATTACHED_OBJECT(klass);

        if (CLASS_OR_MODULE_P(v)) {
            rb_str_append(s, rb_inspect(v));
--- a/proc.c
+++ b/proc.c
@ -56,8 +56,6 @@ static int method_arity(VALUE);
 static int method_min_max_arity(VALUE, int *max);
 static VALUE proc_binding(VALUE self);

-#define attached id__attached__
-
 /* Proc */

 #define IS_METHOD_PROC_IFUNC(ifunc) ((ifunc)->func == bmcall)
@ -1946,7 +1944,7 @@ rb_method_name_error(VALUE klass, VALUE str)
    VALUE s = Qundef;

    if (FL_TEST(c, FL_SINGLETON)) {
-        VALUE obj = rb_ivar_get(klass, attached);
+        VALUE obj = RCLASS_ATTACHED_OBJECT(klass);

        switch (BUILTIN_TYPE(obj)) {
          case T_MODULE:
@ -3116,7 +3114,7 @@ method_inspect(VALUE method)
        rb_str_buf_append(str, rb_inspect(defined_class));
    }
    else if (FL_TEST(mklass, FL_SINGLETON)) {
-        VALUE v = rb_ivar_get(mklass, attached);
+        VALUE v = RCLASS_ATTACHED_OBJECT(mklass);

        if (UNDEF_P(data->recv)) {
            rb_str_buf_append(str, rb_inspect(mklass));
@ -3136,7 +3134,7 @@ method_inspect(VALUE method)
    else {
        mklass = data->klass;
        if (FL_TEST(mklass, FL_SINGLETON)) {
-            VALUE v = rb_ivar_get(mklass, attached);
+            VALUE v = RCLASS_ATTACHED_OBJECT(mklass);
            if (!(RB_TYPE_P(v, T_CLASS) || RB_TYPE_P(v, T_MODULE))) {
                do {
                   mklass = RCLASS_SUPER(mklass);
--- a/variable.c
+++ b/variable.c
@ -3582,7 +3582,7 @@ static VALUE
 cvar_front_klass(VALUE klass)
 {
    if (FL_TEST(klass, FL_SINGLETON)) {
-        VALUE obj = rb_ivar_get(klass, id__attached__);
+        VALUE obj = RCLASS_ATTACHED_OBJECT(klass);
        if (rb_namespace_p(obj)) {
            return obj;
        }
@ -3801,7 +3801,7 @@ mod_cvar_of(VALUE mod, void *data)
 {
    VALUE tmp = mod;
    if (FL_TEST(mod, FL_SINGLETON)) {
-        if (rb_namespace_p(rb_ivar_get(mod, id__attached__))) {
+        if (rb_namespace_p(RCLASS_ATTACHED_OBJECT(mod))) {
            data = mod_cvar_at(tmp, data);
            tmp = cvar_front_klass(tmp);
        }
--- a/vm.c
+++ b/vm.c
@ -12,6 +12,7 @@

 #include "eval_intern.h"
 #include "internal.h"
+#include "internal/class.h"
 #include "internal/compile.h"
 #include "internal/cont.h"
 #include "internal/error.h"
@ -542,7 +543,7 @@ rb_dtrace_setup(rb_execution_context_t *ec, VALUE klass, ID id,
        klass = RBASIC(klass)->klass;
    }
    else if (FL_TEST(klass, FL_SINGLETON)) {
-        klass = rb_attr_get(klass, id__attached__);
+        klass = RCLASS_ATTACHED_OBJECT(klass);
        if (NIL_P(klass)) return FALSE;
    }
    type = BUILTIN_TYPE(klass);
--- a/vm_backtrace.c
+++ b/vm_backtrace.c
@ -11,6 +11,7 @@

 #include "eval_intern.h"
 #include "internal.h"
+#include "internal/class.h"
 #include "internal/error.h"
 #include "internal/vm.h"
 #include "iseq.h"
@ -1746,7 +1747,7 @@ rb_profile_frame_classpath(VALUE frame)
            klass = RBASIC(klass)->klass;
        }
        else if (FL_TEST(klass, FL_SINGLETON)) {
-            klass = rb_ivar_get(klass, id__attached__);
+            klass = RCLASS_ATTACHED_OBJECT(klass);
            if (!RB_TYPE_P(klass, T_CLASS) && !RB_TYPE_P(klass, T_MODULE))
                return rb_sprintf("#<%s:%p>", rb_class2name(rb_obj_class(klass)), (void*)klass);
        }
--- a/vm_method.c
+++ b/vm_method.c
@ -19,7 +19,6 @@ static inline rb_method_entry_t *lookup_method_table(VALUE klass, ID id);
 #define singleton_removed   idSingleton_method_removed
 #define undefined           idMethod_undefined
 #define singleton_undefined idSingleton_method_undefined
-#define attached            id__attached__

 #define ruby_running (GET_VM()->running)
 /* int ruby_running = 0; */
@ -1086,7 +1085,7 @@ check_overloaded_cme(const rb_callable_method_entry_t *cme, const struct rb_call
        VALUE recv_class = (klass);			\
        ID hook_id = (hook);				\
        if (FL_TEST((klass), FL_SINGLETON)) {		\
-            recv_class = rb_ivar_get((klass), attached);	\
+            recv_class = RCLASS_ATTACHED_OBJECT((klass));	\
            hook_id = singleton_##hook;			\
        }						\
        rb_funcallv(recv_class, hook_id, 1, &arg);	\
--- a/vm_trace.c
+++ b/vm_trace.c
@ -23,6 +23,7 @@

 #include "eval_intern.h"
 #include "internal.h"
+#include "internal/class.h"
 #include "internal/hash.h"
 #include "internal/symbol.h"
 #include "iseq.h"
@ -724,7 +725,7 @@ call_trace_func(rb_event_flag_t event, VALUE proc, VALUE self, ID id, VALUE klas
            klass = RBASIC(klass)->klass;
        }
        else if (FL_TEST(klass, FL_SINGLETON)) {
-            klass = rb_ivar_get(klass, id__attached__);
+            klass = RCLASS_ATTACHED_OBJECT(klass);
        }
    }

--- a/yjit/src/cruby_bindings.inc.rs
+++ b/yjit/src/cruby_bindings.inc.rs
@ -1163,6 +1163,9 @@ extern "C" {
    pub fn rb_vm_frame_method_entry(
        cfp: *const rb_control_frame_t,
    ) -> *const rb_callable_method_entry_t;
+    pub fn rb_gvar_get(arg1: ID) -> VALUE;
+    pub fn rb_gvar_set(arg1: ID, arg2: VALUE) -> VALUE;
+    pub fn rb_ensure_iv_list_size(obj: VALUE, len: u32, newsize: u32);
    pub fn rb_obj_as_string_result(str_: VALUE, obj: VALUE) -> VALUE;
    pub fn rb_str_concat_literals(num: usize, strary: *const VALUE) -> VALUE;
    pub fn rb_ec_str_resurrect(ec: *mut rb_execution_context_struct, str_: VALUE) -> VALUE;
@ -1178,9 +1181,6 @@ extern "C" {
        key: st_data_t,
        pval: *mut st_data_t,
    ) -> ::std::os::raw::c_int;
-    pub fn rb_gvar_get(arg1: ID) -> VALUE;
-    pub fn rb_gvar_set(arg1: ID, arg2: VALUE) -> VALUE;
-    pub fn rb_ensure_iv_list_size(obj: VALUE, len: u32, newsize: u32);
    pub fn rb_vm_insn_decode(encoded: VALUE) -> ::std::os::raw::c_int;
    pub fn rb_vm_insn_addr2opcode(addr: *const ::std::os::raw::c_void) -> ::std::os::raw::c_int;
    pub fn rb_iseq_line_no(iseq: *const rb_iseq_t, pos: usize) -> ::std::os::raw::c_uint;