GNOME Bugzilla – Bug 657992
Add glib__private__() API to share between glib,gio; port GWakeup to it
Last modified: 2011-09-09 19:21:50 UTC
Historically we've added random symbols to the public API with warnings that they're private; examples are: glib_gettext(), glib_pgettext() g_thread_functions_for_glib_use, g_thread_use_default_impl, etc. And we almost added "GWakeup" to public API just to share between glib and gio. This new glib__private__() API exports a hidden vtable, and adds a macro GLIB_PRIVATE_CALL() that makes it generally convenient to use. This adds an extremely tiny cost for the double indirection; but it has the benefit that we don't need to either: 1) compile the code into both glib and gio (like GWakeup) 2) Export a poorly named "do not use this" symbol
Created attachment 195427 [details] [review] Add glib__private__() API to share between glib,gio; port GWakeup to it
I have to admit that I don't really like this idea. I'm not excessively concerned about glib_ symbols appearing in our ABI, even thought I know it might cause grief to certain distributors if they disappear or change later. We just need to explain to them that what they think is a problem is not really a problem.
(In reply to comment #2) > I have to admit that I don't really like this idea. Why? What's the concrete problem? > I'm not excessively concerned about glib_ symbols appearing in our ABI, even > thought I know it might cause grief to certain distributors if they disappear > or change later. We just need to explain to them that what they think is a > problem is not really a problem. I really want to support the LSB ABI people. Actually one thing that's more important is that - particularly if we document it - people outside of glib might actually use it. Which we don't want.
Created attachment 195449 [details] [review] Add glib__private__() API to share between glib,gio; port GWakeup to it Don't drop gwakeup.h from sources
Review of attachment 195449 [details] [review]: I have to admit that I generally like the idea of not having two copies of GWakeup. On the other hand I'm concerned by the very premise of this. When faced with the fact that there is a tool that attempts to detect ABI changes, we workaround the fact by moving from one type of ABI change (symbol appear/disappearance) to another type (structure definition changes) simply because we believe it's harder to detect the second one. One possible suggestion, if we do this, would be to split it out into its own .c file. That way we don't have to pile a whole bunch of random #include at the top of gutils.c for all the various private bits we intend to expose. ::: glib/gutils.c @@ +206,3 @@ + &g_wakeup_get_pollfd, + &g_wakeup_signal, + &g_wakeup_acknowledge we usually don't use & when taking the address of functions ::: glib/gutils.h @@ +37,3 @@ G_BEGIN_DECLS +gpointer glib__private__ (int unused); why the unused int parameter?
(In reply to comment #5) > > to another type (structure definition changes) simply > because we believe it's harder to detect the second one. No - the structure is *not public*. glib-private.h is not installed. The exposed API is only "gpointer". > One possible suggestion, if we do this, would be to split it out into its own > .c file. That way we don't have to pile a whole bunch of random #include at > the top of gutils.c for all the various private bits we intend to expose. Good idea, will do. > ::: glib/gutils.c > @@ +206,3 @@ > + &g_wakeup_get_pollfd, > + &g_wakeup_signal, > + &g_wakeup_acknowledge > > we usually don't use & when taking the address of functions Will fix. > ::: glib/gutils.h > @@ +37,3 @@ > G_BEGIN_DECLS > > +gpointer glib__private__ (int unused); > > why the unused int parameter? I was thinking that just in case we needed flags or something. Maybe I'll change it to a pointer which is even more extensible.
Created attachment 195481 [details] [review] Add glib__private__() API to share between glib,gio; port GWakeup to it Updated for comments
(In reply to comment #6) > No - the structure is *not public*. glib-private.h is not installed. The > exposed API is only "gpointer". This is the same argument I make for existing glib_* functions not being public. We don't install the headers.
Actually, it's weird that you have glib__private__() declaration in a public header at all. That should be in glib-private.h as well, and it should just return the pointer type directly. From an ABI standpoint, it's not possible to detect the difference, and this way we don't get a weird looking function appearing in a public (and installed) header. About the argument: since we're free to change the ABI of this function, we don't need it. If we come up with a future usecase for it, we can change it. ...unless the LSB ABI tools are more powerful than I know to be possible.
(In reply to comment #8) > (In reply to comment #6) > > No - the structure is *not public*. glib-private.h is not installed. The > > exposed API is only "gpointer". > > This is the same argument I make for existing glib_* functions not being > public. We don't install the headers. Yeah, true. I guess then one notable difference with this approach for e.g. glib_gettext is that on a 32 bit system, you can call glib_gettext() (maybe you see the function name in some glib sources) and you'll get just a compiler warning - it'll actually work. Calling glib__private__() would be much harder and it's just much more obvious that it's private. (In reply to comment #9) > Actually, it's weird that you have glib__private__() declaration in a public > header at all. That should be in glib-private.h as well, and it should just > return the pointer type directly. You are totally right, will fix. > ...unless the LSB ABI tools are more powerful than I know to be possible. The difference will show up in debuginfo. To make this more concrete: http://ispras.linux-foundation.org/index.php/ABI_compliance_checker#Detectable_Compatibility_Problems
Created attachment 195524 [details] [review] Add glib__private__() API to share between glib,gio; port GWakeup to it Move into private header
Created attachment 196138 [details] [review] glib worker: move to glib-private framework Remove the private glib_get_worker_context() symbol and move it over to using the glib-private stuff like GWakeup is doing.