MRI: {Rect,Color,Tone}#initialize_copy instead of #clone #3

Closed

cremno wants to merge 1 commits from mri-use-initialize_copy into master

cremno commented

2013-10-22 15:41:19 +00:00

(Migrated from github.com)

removed unused CLONE_FUNC
#initialize_copy should be used instead of #clone. It's the general way to do it and the RGSS defines it, too

Or was there a reason for directly defining #cloneinstead?

(Your code is weird. :3 Well, the RGSS is too but not in that way. And I don't mean the coding style! Rather stuff like setPrivateData. Why are two objects used? One object and one ivar called PRIV_IV which holds the data pointer, even though you could just use one object for that.)

- removed unused CLONE_FUNC - #initialize_copy should be used instead of #clone. It's the general way to do it and the RGSS defines it, too Or was there a reason for directly defining #cloneinstead? (Your code is weird. :3 Well, the RGSS is too but not in that way. And I don't mean the coding style! Rather stuff like `setPrivateData`. Why are two objects used? One object and one ivar called PRIV_IV which holds the data pointer, even though you could just use one object for that.)

Ancurio commented

2013-10-22 16:16:45 +00:00

(Migrated from github.com)

Or was there a reason for directly defining #cloneinstead?

Nope, just me having no idea wtf I was doing.

Rather stuff like setPrivateData. Why are two objects used? One object and one ivar called PRIV_IV which holds the data pointer, even though you could just use one object for that.

Hmm. Aside from my inexperience with anything ruby, I think I was put off by exposing "RUBY_T_DATA" directly to the scripts (somehow I thought it'd be more correct to just present an opaque RUBY_T_OBJECT value). Also, I wasn't really sure how initialization would be handled (once again inexperience, never worked on anything Ruby from the C side before mkxp). Looking at mrb's time gem, I guess the right way to do would be to use something like MRB_SET_INSTANCE_TT(klass, MRB_TT_DATA) and then ruby would already have created a Data object whose pointer we could directly set in the initialize method (right?).

Thankfully it looks like we could mostly change this as an implementation detail of {get,set}PrivateData without any major overhauling. But I think that's off topic for this pull request. If you have more questions about the code, feel free to just open an issue.

On topic: Yeah, that looks better than what I had. But I think you forgot Bitmap which also needs this. Also see my inline comment on the commit.

Also, are you compiling mkxp without problems?

> Or was there a reason for directly defining #cloneinstead? Nope, just me having no idea wtf I was doing. > Rather stuff like setPrivateData. Why are two objects used? One object and one ivar called PRIV_IV which holds the data pointer, even though you could just use one object for that. Hmm. Aside from my inexperience with anything ruby, I think I was put off by exposing "RUBY_T_DATA" directly to the scripts (somehow I thought it'd be more correct to just present an opaque RUBY_T_OBJECT value). Also, I wasn't really sure how initialization would be handled (once again inexperience, never worked on anything Ruby from the C side before mkxp). Looking at mrb's time gem, I guess the right way to do would be to use something like `MRB_SET_INSTANCE_TT(klass, MRB_TT_DATA)` and then ruby would already have created a Data object whose pointer we could directly set in the initialize method (right?). Thankfully it looks like we could mostly change this as an implementation detail of {get,set}PrivateData without any major overhauling. But I think that's off topic for this pull request. If you have more questions about the code, feel free to just open an issue. On topic: Yeah, that looks better than what I had. But I think you forgot Bitmap which also needs this. Also see my inline comment on the commit. Also, are you compiling mkxp without problems?

Ancurio commented

2013-10-31 08:58:07 +00:00

(Migrated from github.com)

I needed to fix something that required this init_copy stuff for the Font class, so I just went ahead and incorporated your changes.