GNOME Bugzilla – Bug 719453
Change g_variant_get_string() to take one argument
Last modified: 2018-05-24 16:08:12 UTC
Popular opinion is in: the fact that g_variant_get_string() takes two arguments is the worst API mistake that I've ever made. I think we can fix it. There are two solutions here. The easy way and the hard way. The easy way involves straight deprecation of g_variant_get_string() and introducing something new like g_variant_get_str(). I don't like that because it would make the API inconsistent (vs _new_string() and other functions like _get_boolean(), not _bool(), for example). We could try to keep the existing name with a bit of trickery and some time... Consider introducing g_variant_get_string_with_length() and adding: #ifndef G_DISABLE_DEPRECATED #define PRIVATE_g_variant_get_string(str, len, ...) (g_variant_get_string_with_length((str), (len))) #define g_variant_get_string(...) (PRIVATE_g_variant_get_string(__VA_ARGS__, NULL)) #else #define g_variant_get_string(str) (g_variant_get_string (str, NULL)) #endif This would have the immediate effect of: 1) ABI is completely preserved -- g_variant_get_string() would continue to handle its second argument 2) if G_DISABLE_DEPRECATED then trying to use that second argument would fail 3) all newly-compiled programs would only ever call g_variant_get_string() with NULL for the second argument (at the ABI level) After a while (year or two?...) when we suspect that everyone who has ever used this API with a non-NULL second argument (which is a pretty small proportion to begin with) has recompiled their program, we could add a critical for the case that we see non-NULL second argument. After another cycle we could step that up to an assert with an easy way to disable it to help people work around issues. That's the real ABI break. Another cycle and we could change the prototype, relying (as we do many other places) that extra arguments passed to functions are ignored with no ill effects. It will always be NULL by this point anyway, so it's OK. At that point we can also drop the macro for the G_DISABLE_DEPRECATED case. Note: throughout, API compatibility is preserved as long as G_DISABLE_DEPRECATED is not defined. We only break ABI after waiting a few years for everyone to recompile. Note also: to clarify the above, even if people don't modify their source at all, simply recompiling with the above macros will move them over to compatibility with the final ABI. Thoughts? Note: I tried to implement this unconditionally using this trickery some tricks involving passing 0x1 instead of NULL as the second argument for the case where the caller had one argument and using __builtin_constant_p() and __builtin_choose_expr() to call a deprecated function in case the caller gave a second argument, but there are a pair of upstream gcc bugs that prevent this from working, so it seems that the G_DISABLE_DEPRECATED approach is the best we can do for now: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46073 http://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
Created attachment 262987 [details] [review] GVariant: g_variant_get_string() now takes one arg Use some macro tricks so that g_variant_get_string() will accept either 1 or 2 arguments, and deprecate the case of calling it with two arguments. Add g_variant_get_string_with_length() as a non-deprecated way of getting the length if it is needed. Do the same for g_variant_dup_string() as well, but don't introduce a _with_length() variant.
Created attachment 262988 [details] [review] misc: don't pass 2 args to g_variant_get_string() This function only takes one argument now. In most cases, the second argument was NULL anyway, so just remove it. In a few cases, we use g_variant_get_string_with_length() instead. All use of g_variant_get_string() from inside of GLib was from within GIO except for the uses from inside of GVariant itself (in gvariant.c).
This trickery doesn't make me super-comfortable. Here is an alternative set of tricks that has the effect of defining g_variant_get_string as a macro that will be expanded as follows: g_variant_get_string (v) --> (g_variant_get_string) (v, NULL) g_variant_get_string (v, l) --> (g_variant_get_string) (v, l) #define VA_NUM_ARGS(...) VA_NUM_ARGS_IMPL(__VA_ARGS__, 5,4,3,2,1) #define VA_NUM_ARGS_IMPL(_1,_2,_3,_4,_5,N,...) N #define macro_dispatcher(func, ...) macro_dispatcher_(func, VA_NUM_ARGS(__VA_ARGS__)) #define macro_dispatcher_(func, nargs) macro_dispatcher__(func, nargs) #define macro_dispatcher__(func, nargs) func ## nargs #define g_variant_get_string(...) macro_dispatcher(g_variant_get_string, __VA_ARGS__)(__VA_ARGS__) #define g_variant_get_string1(v) (g_variant_get_string)(v,NULL) #define g_variant_get_string2(v,l) (g_variant_get_string)(v,l) Of course, this will need similar PRIVATE_ namespacing to your solution. Mostly gleaned from http://efesx.com/2010/07/17/variadic-macro-to-count-number-of-arguments/
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/798.