From a35f5be0e8f2a5b1d068c32d8802dabb96a8db47 Mon Sep 17 00:00:00 2001 From: Kouji Takao Date: Mon, 30 Mar 2026 00:26:10 +0900 Subject: [PATCH] fix: address review comments on TypedData migration - Add "ruby-sdl2/" prefix to type names in DEFINE_DATA_TYPE macro - Use RUBY_TYPED_FREE_IMMEDIATELY flag instead of 0 - Replace ALLOC + TypedData_Wrap_Struct with TypedData_Make_Struct Co-Authored-By: Claude Opus 4.6 (1M context) --- event.c | 14 +++++++------ gamecontroller.c.m4 | 5 +++-- gl.c.m4 | 5 +++-- joystick.c.m4 | 5 +++-- mixer.c.m4 | 10 ++++++---- rubysdl2_internal.h | 4 ++-- ttf.c.m4 | 5 +++-- video.c.m4 | 48 ++++++++++++++++++++++----------------------- 8 files changed, 52 insertions(+), 44 deletions(-) diff --git a/event.c b/event.c index d224b5a..d688b2b 100644 --- a/event.c +++ b/event.c @@ -90,9 +90,10 @@ DEFINE_DATA_TYPE(SDL_Event, free); static VALUE Event_new(SDL_Event* ev) { - SDL_Event* e = ALLOC(SDL_Event); + SDL_Event* e; + VALUE obj = TypedData_Make_Struct(event_type_to_class[ev->type], SDL_Event, &SDL_Event_data_type, e); *e = *ev; - return TypedData_Wrap_Struct(event_type_to_class[ev->type], &SDL_Event_data_type, e); + return obj; } static VALUE Event_s_allocate(VALUE klass) @@ -102,10 +103,11 @@ static VALUE Event_s_allocate(VALUE klass) if (event_type == Qnil) rb_raise(rb_eArgError, "Cannot allocate %s", rb_class2name(klass)); - e = ALLOC(SDL_Event); - memset(e, 0, sizeof(SDL_Event)); - e->common.type = NUM2INT(event_type); - return TypedData_Wrap_Struct(klass, &SDL_Event_data_type, e); + { + VALUE obj = TypedData_Make_Struct(klass, SDL_Event, &SDL_Event_data_type, e); + e->common.type = NUM2INT(event_type); + return obj; + } } /* diff --git a/gamecontroller.c.m4 b/gamecontroller.c.m4 index a66ac77..067e0ca 100644 --- a/gamecontroller.c.m4 +++ b/gamecontroller.c.m4 @@ -62,9 +62,10 @@ DEFINE_DATA_TYPE(GameController, GameController_free); static VALUE GameController_new(SDL_GameController* controller) { - GameController* g = ALLOC(GameController); + GameController* g; + VALUE obj = TypedData_Make_Struct(cGameController, GameController, &GameController_data_type, g); g->controller = controller; - return TypedData_Wrap_Struct(cGameController, &GameController_data_type, g); + return obj; } DEFINE_WRAPPER(SDL_GameController, GameController, controller, cGameController, diff --git a/gl.c.m4 b/gl.c.m4 index 36bb86f..06431d6 100644 --- a/gl.c.m4 +++ b/gl.c.m4 @@ -25,9 +25,10 @@ DEFINE_WRAPPER(SDL_GLContext, GLContext, context, cGLContext, "SDL2::GL::Context static VALUE GLContext_new(SDL_GLContext context) { - GLContext* c = ALLOC(GLContext); + GLContext* c; + VALUE obj = TypedData_Make_Struct(cGLContext, GLContext, &GLContext_data_type, c); c->context = context; - return TypedData_Wrap_Struct(cGLContext, &GLContext_data_type, c); + return obj; } /* diff --git a/joystick.c.m4 b/joystick.c.m4 index b1f6fa3..6b28fef 100644 --- a/joystick.c.m4 +++ b/joystick.c.m4 @@ -22,9 +22,10 @@ DEFINE_DATA_TYPE(Joystick, Joystick_free); static VALUE Joystick_new(SDL_Joystick* joystick) { - Joystick* j = ALLOC(Joystick); + Joystick* j; + VALUE obj = TypedData_Make_Struct(cJoystick, Joystick, &Joystick_data_type, j); j->joystick = joystick; - return TypedData_Wrap_Struct(cJoystick, &Joystick_data_type, j); + return obj; } DEFINE_WRAPPER(SDL_Joystick, Joystick, joystick, cJoystick, "SDL2::Joystick"); diff --git a/mixer.c.m4 b/mixer.c.m4 index 4943f8f..9a1e66d 100644 --- a/mixer.c.m4 +++ b/mixer.c.m4 @@ -36,9 +36,10 @@ DEFINE_DATA_TYPE(Chunk, Chunk_free); static VALUE Chunk_new(Mix_Chunk* chunk) { - Chunk* c = ALLOC(Chunk); + Chunk* c; + VALUE obj = TypedData_Make_Struct(cChunk, Chunk, &Chunk_data_type, c); c->chunk = chunk; - return TypedData_Wrap_Struct(cChunk, &Chunk_data_type, c); + return obj; } DEFINE_WRAPPER(Mix_Chunk, Chunk, chunk, cChunk, "SDL2::Mixer::Chunk"); @@ -54,9 +55,10 @@ DEFINE_DATA_TYPE(Music, Music_free); static VALUE Music_new(Mix_Music* music) { - Music* c = ALLOC(Music); + Music* c; + VALUE obj = TypedData_Make_Struct(cMusic, Music, &Music_data_type, c); c->music = music; - return TypedData_Wrap_Struct(cMusic, &Music_data_type, c); + return obj; } DEFINE_WRAPPER(Mix_Music, Music, music, cMusic, "SDL2::Mixer::Music"); diff --git a/rubysdl2_internal.h b/rubysdl2_internal.h index b388595..b5b0d05 100644 --- a/rubysdl2_internal.h +++ b/rubysdl2_internal.h @@ -77,9 +77,9 @@ void rubysdl2_init_gamecontorller(void); */ #define DEFINE_DATA_TYPE(struct_name, free_func) \ static const rb_data_type_t struct_name##_data_type = { \ - #struct_name, \ + "ruby-sdl2/" #struct_name, \ { NULL, (void (*)(void*))(free_func), NULL }, \ - NULL, NULL, 0 \ + NULL, NULL, RUBY_TYPED_FREE_IMMEDIATELY \ }; #define DEFINE_GETTER(scope, ctype, var_class, classname) \ diff --git a/ttf.c.m4 b/ttf.c.m4 index 3f82b33..6f112f3 100644 --- a/ttf.c.m4 +++ b/ttf.c.m4 @@ -47,9 +47,10 @@ DEFINE_DATA_TYPE(TTF, TTF_free); static VALUE TTF_new(TTF_Font* font) { - TTF* f = ALLOC(TTF); + TTF* f; + VALUE obj = TypedData_Make_Struct(cTTF, TTF, &TTF_data_type, f); f->font = font; - return TypedData_Wrap_Struct(cTTF, &TTF_data_type, f); + return obj; } DEFINE_WRAPPER(TTF_Font, TTF, font, cTTF, "SDL2::TTF"); diff --git a/video.c.m4 b/video.c.m4 index 284ad52..85d6c9b 100644 --- a/video.c.m4 +++ b/video.c.m4 @@ -102,12 +102,12 @@ static void Window_free(Window* w) static VALUE Window_new(SDL_Window* window) { - Window* w = ALLOC(Window); + Window* w; + VALUE obj = TypedData_Make_Struct(cWindow, Window, &Window_data_type, w); w->window = window; - w->num_renderers = 0; w->max_renderers = 4; w->renderers = ALLOC_N(struct Renderer*, 4); - return TypedData_Wrap_Struct(cWindow, &Window_data_type, w); + return obj; } DEFINE_GETTER(static, Window, cWindow, "SDL2::Window"); @@ -124,10 +124,9 @@ static VALUE Display_new(int index) static VALUE DisplayMode_s_allocate(VALUE klass) { - SDL_DisplayMode* mode = ALLOC(SDL_DisplayMode); - mode->format = mode->w = mode->h = mode->refresh_rate = 0; - mode->driverdata = NULL; - return TypedData_Wrap_Struct(klass, &SDL_DisplayMode_data_type, mode); + SDL_DisplayMode* mode; + VALUE obj = TypedData_Make_Struct(klass, SDL_DisplayMode, &SDL_DisplayMode_data_type, mode); + return obj; } static VALUE DisplayMode_new(SDL_DisplayMode* mode) @@ -179,14 +178,14 @@ static void Window_attach_renderer(Window* w, Renderer* r) static VALUE Renderer_new(SDL_Renderer* renderer, Window* w) { - Renderer* r = ALLOC(Renderer); + Renderer* r; + VALUE obj = TypedData_Make_Struct(cRenderer, Renderer, &Renderer_data_type, r); r->renderer = renderer; - r->num_textures = 0; r->max_textures = 16; r->textures = ALLOC_N(Texture*, 16); r->refcount = 1; Window_attach_renderer(w, r); - return TypedData_Wrap_Struct(cRenderer, &Renderer_data_type, r); + return obj; } DEFINE_WRAPPER(SDL_Renderer, Renderer, renderer, cRenderer, "SDL2::Renderer"); @@ -221,11 +220,12 @@ static void Renderer_attach_texture(Renderer* r, Texture* t) static VALUE Texture_new(SDL_Texture* texture, Renderer* r) { - Texture* t = ALLOC(Texture); + Texture* t; + VALUE obj = TypedData_Make_Struct(cTexture, Texture, &Texture_data_type, t); t->texture = texture; t->refcount = 1; Renderer_attach_texture(r, t); - return TypedData_Wrap_Struct(cTexture, &Texture_data_type, t); + return obj; } DEFINE_WRAPPER(SDL_Texture, Texture, texture, cTexture, "SDL2::Texture"); @@ -243,10 +243,10 @@ static void Surface_free(Surface* s) VALUE Surface_new(SDL_Surface* surface) { - Surface* s = ALLOC(Surface); + Surface* s; + VALUE obj = TypedData_Make_Struct(cSurface, Surface, &Surface_data_type, s); s->surface = surface; - s->need_to_free_pixels = 0; - return TypedData_Wrap_Struct(cSurface, &Surface_data_type, s); + return obj; } DEFINE_WRAPPER(SDL_Surface, Surface, surface, cSurface, "SDL2::Surface"); @@ -2106,6 +2106,7 @@ static VALUE Surface_s_from_string(int argc, VALUE* argv, VALUE self) SDL_Surface* surface; void* pixels; Surface* s; + VALUE obj; rb_scan_args(argc, argv, "45", &string, &width, &height, &depth, &pitch, &Rmask, &Gmask, &Bmask, &Amask); @@ -2132,10 +2133,10 @@ static VALUE Surface_s_from_string(int argc, VALUE* argv, VALUE self) RB_GC_GUARD(string); - s = ALLOC(Surface); + obj = TypedData_Make_Struct(cSurface, Surface, &Surface_data_type, s); s->surface = surface; s->need_to_free_pixels = 1; - return TypedData_Wrap_Struct(cSurface, &Surface_data_type, s); + return obj; } /* @@ -2535,10 +2536,9 @@ static VALUE Surface_s_new(int argc, VALUE* argv, VALUE self) */ static VALUE Rect_s_allocate(VALUE klass) { - SDL_Rect* rect = ALLOC(SDL_Rect); - rect->x = rect->y = rect->w = rect->h = 0; - - return TypedData_Wrap_Struct(cRect, &SDL_Rect_data_type, rect); + SDL_Rect* rect; + VALUE obj = TypedData_Make_Struct(klass, SDL_Rect, &SDL_Rect_data_type, rect); + return obj; } /* @@ -2640,9 +2640,9 @@ static VALUE Rect_union(VALUE self, VALUE other) */ static VALUE Point_s_allocate(VALUE klass) { - SDL_Point* point = ALLOC(SDL_Point); - memset(point, 0, sizeof(SDL_Point)); - return TypedData_Wrap_Struct(klass, &SDL_Point_data_type, point); + SDL_Point* point; + VALUE obj = TypedData_Make_Struct(klass, SDL_Point, &SDL_Point_data_type, point); + return obj; } /*