probable deprecation, soon (might be already done) of Data_Wrap_Struct/Data_Get_Struct

[passenger94]

https://github.com/ruby/ruby/blob/6a69ab937c7e6584d311cd444615cc1cd5b5499b/doc/extension.rdoc#encapsulate-c-data-into-a-ruby-object

– The old (non-Typed) Data_XXX macro family has been deprecated. In the future version of Ruby, it is possible old macros will not work. ++

This will impact every corner of C Shoes ...
We have to switch to the new API : TypedData_Wrap_Struct / TypedData_Get_Struct

Lots of reviewing but hopefully nothing harmful ...

Not related i think ...
i can compile Shoes against ruby 2.3.1 but fails to launch somewhere in rb_lod_path ... (just me ?)

[ccoupe]

I haven't tried 2.3. There is/was some hardcoded nonsense in cache.rb/shoes.rb that might need adjusting. and some rakefiles.

I'm not surprised that the Ruby Gods are moving the goal posts until no-one can embed Ruby. I should probably rejoin the Ruby ML.

[passenger94]

well, the "new APi" is there since 1.9 apparently, so Gods have been patient this time :-D

[passenger94]

made some tests, i think i can handle it, it works on video widget, still some tiny Macro to think about,to be as generic as possible ...
Your beloved Dragons are lying in every existing Macro which might have a "hidden" Data_Get_Struct,
Gigantic boring copy/paste orgy in sight ...

Basically :
define a dsize function (fetch the size of the wrapped Struct) and a rb_data_type_t struct for every underlying C struct of a ruby object
in alloc function change accordingly from Data_Wrap_Struct to TypedData_Wrap_Struct
everywhere change Data_Get_Struct to TypedData_Get_Struct
for video widget :

static size_t video_dsize(const void *vid)
{
    return sizeof(shoes_video);
}

const rb_data_type_t shoes_video_type = {
    "video_type",
    {
      (void (*)(void *))shoes_video_mark, 
      (void (*)(void *))shoes_video_free, 
      video_dsize,
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY, // <--- this one needs your thoughts
};

VALUE shoes_video_alloc(VALUE klass) {
  shoes_video *video = SHOE_ALLOC(shoes_video);
  SHOE_MEMZERO(video, shoes_video, 1);

//  VALUE obj = Data_Wrap_Struct(klass, shoes_video_mark, shoes_video_free, video);
  VALUE obj = TypedData_Wrap_Struct(klass, &shoes_video_type, video);
  video->attr = Qnil;
  video->parent = Qnil;
  video->realized = 0;
  return obj;
}

VALUE
shoes_video_get_realized(VALUE self) {
  shoes_video *self_t;
//  Data_Get_Struct(self, shoes_video, self_t);
  TypedData_Get_Struct(self, shoes_video, &shoes_video_type, self_t);

  return self_t->realized ? Qtrue : Qfalse;
}

PS : it's deprecated since 2.2 !

[ccoupe]

Lets think very hard before doing anything that would cause such a massive git merge (doubling the cut/paste work for all the conflicts). I believe C can spit out the size of a struct without calling a function that call sizeof. That might simplify some things.

[passenger94]

sure, working on a separate branch, merging when really confident !
ha yes, most people are just issuing a 0 in place of the dsize

[ccoupe]

More issues that just a different branch. it's a freeze on any branch that has a conflict. If the rewrite is to the old Ruby style instead of the new gc write barrier stuff, can we use both old and new style, widget by widget, function by function?

[passenger94]

the funny thing is that you can access the underlying struct directly now, bypassing the TypedData_Get_Struct, which as i understand it is supposed to be safer :
there is a RTYPEDDATA_DATA(rubyobject) Macro for that .... returning a void pointer to the struct

[passenger94]

oh, no need for write barrier, only if you want the new gc generational feature

[passenger94]

yes we can mix both styles, that was my plan, going slowly widget by widget, fix problems, commit, move on

There is some real fun ! for example in shoes_canvas_draw function you have to guard (check if ele is a cCanvas) before calling TypedData_Get_Struct(ele, shoes_canvas, c1); otherwise it won't work, in the old api, the check was done by the Macro .... >_> (took me a while to figure out this one)

[ccoupe]

If we can mix them then it does make things easier. Just co-ordinate which widgets are being upgraded so we don't collide. BTW sizeof(type) is a C operator so this should work

const rb_data_type_t shoes_video_type = {
    "video_type",
    {
      (void (*)(void *))shoes_video_mark, 
      (void (*)(void *))shoes_video_free, 
      sizeof(shoes_video)
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY, // <--- this one needs your thoughts
}

My understanding of the doc is that RUBY_TYPED_FREE_IMMEDIATELY is the backwards compatible method, the one we want.

[ccoupe]

I'm going to merge the graph branch into master even though the plot widget is only 80% complete (but semi-usable) and it has the #240 fixes; You can then merge master into your branch and later collisions will be minimized, I hope. I added one more method to plot and 'll try the new api on it and get some experience.

[passenger94]

yes, but someone wants a cast ! ... this works on video widget

const rb_data_type_t shoes_video_type = {
    "video_type",
    {(void (*)(void *))shoes_video_mark, 
      (void (*)(void *))shoes_video_free, 
      (size_t (*)(const void *))sizeof(shoes_video),
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY,
};

let's go with RUBY_TYPED_FREE_IMMEDIATELY (my understanding is this tells ruby to free the data_type_t struct as soon as the shoes_video struct is free-ed - ie not waiting a GC collection - so it looks adequate.

Maybe there is no emergency to integrate plot into master (in case you want to quietly polish your creation) : we are going to learn a lot about the real implications of this change, so later it could ease the integration, knowing where to look for problems, just a thought ...

[passenger94]

we need to put redirection flow in some methods, like shoes_basic_remove, the time of transition, there is a Macro which checks if an object is Typed or not : RTYPEDDATA_P(rubyobject)

SETUP_BASiC Macro needs a revamp, because we can't access shoes_basic struct like before
this looks working good :
shoes_basic* basic = (shoes_basic*)RTYPEDDATA_DATA(rubyobject)
and we can access parent, attr members like in old api

[ccoupe]

#define VIDEO_STRUCT_SZ (size_t (*)(const void *))sizeof(shoes_video)
const rb_data_type_t shoes_video_type = {
    "video_type",
    {(void (*)(void *))shoes_video_mark, 
      (void (*)(void *))shoes_video_free, 
     VIDEO_STRUCT_SZ ,
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY,
};

Only a thought. Too late on the merge to master. I'm going to let you figure out the new api on your widget of choice while put the finishing touch on plot with the old api.

[passenger94]

#define TYPED_STRUCT_SZ(base)  (size_t (*)(const void *))sizeof(base)

not tested yet

[ccoupe]

Very good.

[passenger94]

created a typeddata branch on my repository,
it almost works on video widget, except some glitches while drawing, noticeable on the volume knob (doesn't refresh correctly) and video tests are failing erratically, i suspect some Macro somewhere which doesn’t appreciate avant-garde experiments...

[passenger94]

hello,
If you have some time, could you check, please, names, macros and the general idea, before i go too far, shoes_video and shoes_app are done, and of course as usual there is cocoa...(changes are there with commented ancient code).
no hurry

[ccoupe]

If you create a remote branch and push to it, then I can pull your changes down.

[passenger94]

yep, on my repository, updated to your latest commits now : branch typeddata
(did you mean creating one on your repository ?)

[ccoupe]

Yeah, I was expecting the new branch on my repo. I'm pretty sure I do not want to do a simple clone your repo - that would be confusing for me.

[passenger94]

Hello @ccoupe, did you got a chance to look at the proposed changes ? i might have some times to continue ...

[ccoupe]

Haven't had the chance. I keep finding errors with my chart code ;-) And moving goal posts.

[ccoupe]

Video tests, for me, report the same volume control err they did on the master branch. I don't see any new problems with video for the typeddate branch. Svg might be a place to try your new macros on. Do you know if ffi is using new code or old code?

[passenger94]

Ok i'll tackle that (svg), i just wanted to know if the new Macros are acceptable like they are now (names, functionality, ...), as you obviously know, later it would mean redoing the whole thing ... again...
app was fine too, apparently .... sounds like an approval :-)

Video tests, for me, report the same volume control err

? what is the error.

[passenger94]

forgot to mention, Macros of interest are here :
https://github.com/Shoes3/shoes3/blob/typeddata/shoes/ruby.h#L142

here is a summarize of what is involved :

Calling TypedDATA_type_new(...) for every wrappd struct, which creates the new type struct;
for example with shoes_app : TypedDATA_type_new(shoes_app) creates

const rb_data_type_t shoes_app_type = {
    "shoes_app_type",
    {
      (void (*)(void *))shoes_app_mark,
      (void (*)(void *))shoes_app_free,
      (size_t (*)(const void *))sizeof(shoes_app),
    },
    0, 0,
    RUBY_TYPED_FREE_IMMEDIATELY,
}

then replacing old GET_STRUCTby GET_TypedSTRUCT ........// unwraps implicit self
also old Data_Get_Struct by GET_TypedSTRUCT2 ............// unwraps explicit ruby object
and old Data_Wrap_Struct by TypedData_Wrap_Struct

plus the new way to access beginning part of a struct via struct shoes_basic (talked about in earlier comment, above)

tedious but easy, the hard part is finding hidden side effects (mostly in Macros) ... (and no, i won't complain about missing tests because santa is too far away)

Edit
instead of

  VALUE obj = shoes_svg_alloc(cSvg);
  shoes_svg *self_t;
  Data_Get_Struct(obj, shoes_svg, self_t);

now i propose (less verbose and coherent with GET_STRUCT / GET_TypedSTRUCT)
self_t is declared/assigned inside the Macro

  VALUE obj = shoes_svg_alloc(cSvg);
  GET_TypedSTRUCT2(obj, shoes_svg, self_t);

But while we are at it, personally, i would really like
shoes_svg *self_t = GET_TypedSTRUCT2(obj, shoes_svg);
(that way self_t is not hidden inside the Macro, idem for GET_TypedSTRUCT )

Also
instead of things like

GET_STRUCT(svg, svg);

i propose

GET_TypedSTRUCT(shoes_svg, svg);

type the whole type ! not only part of it, i find it often confusing

[ccoupe]

I was going to complain about the mixed case Macro names but I see that Ruby has decided it's good for them. We can adapt.

I too find the GET_STRUCT(svg, svg) confusing and prefer both of your suggestions.
GET_TypedSTRUCT(shoes_svg, svg); and your shoes_svg *self_t = GET_TypedSTRUCT2(obj, shoes_svg);`

[passenger94]

recap of options for unwrapping Macros : (of course, shoes_svg and svg are just examples, all options compiles and work for me)

A - as of now, 2 Macros (for implicit self and whatever ruby object) hidden declaration/assignment of variable svg inside Macro, less verbose
GET_TypedSTRUCT(shoes_svg, svg);
GET_TypedSTRUCT2(rubyObj, shoes_svg, svg);
... do stuff with svg variable

B - 2 Macros explicit declaration of variable svg
shoes_svg *svg = GET_TypedSTRUCT(shoes_svg);
shoes_svg *svg = GET_TypedSTRUCT2(rubyObj, shoes_svg);
... do stuff with svg variable

C - 1 Macro, tiny bit more verbose in the self case (seen very often in code)
shoes_svg *svg = GET_TypedSTRUCT(self, shoes_svg) )
shoes_svg *svg = GET_TypedSTRUCT(rubyObj, shoes_svg);
... do stuff with svg variable

either A or C for me (looking at this, maybe A is better, finally)

Note, there is some few cases where use of those Macros is not appropriate, for example if we must declare the variable beforehand, there we use the original ruby Macro : TypedData_Get_Struct (instead of old Data_Get_Struct)
!! easy to forget, when looking up for changes to do in code ...

about Macros in all Caps convention, as visual feedback, those would become :
GET_TYPEDSTRUCT
TYPEDDATA_TYPE_NEW

again no obvious choice for me, all Caps has the advantage to disambiguate ruby's Macro from Shoes's one, though

so, it seems i have some proclivity (didn't knew one could have proclivity, hope it's not contagious !) for :
TYPEDDATA_TYPE_NEW(shoes_svg)
GET_TYPEDSTRUCT(shoes_svg, svg);
GET_TYPEDSTRUCT2(rubyObj, shoes_svg, svg);

waiting for your feedback :-)

[passenger94]

Svg is working now, but i discovered that something with the new API interacts badly with the forced garbage collection inside shoes_svg_set_handle, commenting out the call to rb_gc() "fix" the problem but Shoes leaks or disgustingly bleeds i should say ...... (testing with good-cardflip.rb)

I hope it's not related to the new generational gc ...... if so, that's good news for our go back to school plan ....

[ccoupe]

It could be a Shoes problem. In theory, that rb_gc call shouldn't be there. But, it is there for a reason.

Perhaps the svghandle code is holding on to something that the new gc sees as being in use. Animate may also be part of the problem. It leaks little bits. If you left the old wavy line Shoes splash screen running overnight - no memory in the morning.

[ccoupe]

Things have gotten 'complicated' since @passenger94 wrote the macro replacements. That was before the new file scheme @backorder created. Now it is glaringly obvious just how 'inter-twingled' they are. Just about every Macro needs a duplicate and some #ifdef's to select between them. Picking svg or plot is the easiest since they have very little 'twingle' Every file/shoes object will have to be touched at least twice. - Once to do the above, Getting the object to work, and then when all objects are converted and working, removing the #ifdef's. Tedious and it is not as simple as cut and paste or mass batch edits.

[ccoupe]

I had to resort to some trickery to get svg (or any other widget) to draw without hanging/crashing. In file canvas.c: 572 - function shoes_canvas_draw(), it blindly unwraps widgets or any type into a shoes_canvas. That fails big time on objects created with the new macros. This is not the only place in Shoes - I know I did something like that somewhere too. So, for doc purposes, here's a trick to get past the type checking.

            VALUE ele = rb_ary_entry(self_t->contents, i);
            if (RTYPEDDATA_P(ele)) {
              /* This a nasty trick. The original blindly assumes that
               * Data_Get_Struct can unwrap anything into a shoes_canvas.
               * shoes_element would be a better choice but they both fail
               * for TypedData objects
              */
              c1 = (shoes_canvas *)RTYPEDDATA_DATA(ele);
            } else {
              Data_Get_Struct(ele, shoes_canvas, c1);
            }

[ccoupe]

Some good news. Look at the Ruby version!

There is a lot of work required get this to mainstream Shoes but it's a good start.

[dredknight]

Rock on! I hope it does not mess with my code too much. I will fix it xD

[ccoupe]

I tried pushing the edge -Shoes 3.3.8 built on Windows 10, 64bit Ruby 2.5.5 and the latest Gtk 3.24.1. That's a whole lotta new .There are some issues to fix but It works, so far.