GNOME Bugzilla – Bug 793186
GValue – Don't cast G_VALUE_TYPE() argument to GValue*
Last modified: 2018-02-08 14:51:31 UTC
See commit message. There are a few similar things in other macros. Any opinion on this? It can in theory break existing, valid code, e.g. if someone uses a struct { GValue v; OtherStuff o; } and passes that to G_VALUE_TYPE().
Created attachment 367913 [details] [review] GValue – Don't cast G_VALUE_TYPE() argument to GValue* It's not possible to subclass GValue, and by always explicitly casting here it is easy to write broken code (e.g. passing a GValue**) without the compiler warning about that. By not casting, the compiler will error out if anything but a GValue* is passed here.
Review of attachment 367913 [details] [review]: Seems reasonable to me. If callers want to do funny things like `struct { GValue v; OtherStuff s; }` they can cast before passing the argument to G_VALUE_TYPE. This should be OK with GValue and const GValue, which are the two cases I’m most worried about. I’d like to see how this does when building the full stack though. Emmanuele, if we pushed this to GLib master, could we kick off a rebuild of the stack and see what breaks? I’d like to coordinate with you on that.
Sebastian, have you compiled GStreamer against a GLib with this patch, for example?
(In reply to Philip Withnall from comment #2) > I’d like to see how this does when building the full stack though. > Emmanuele, if we pushed this to GLib master, could we kick off a rebuild of > the stack and see what breaks? I’d like to coordinate with you on that. Sure; just tell me when the patch is merged and I'll kick off a full Continuous rebuild for that.
Pushed to master. Emmanuele, can you kick off a rebuild please? I’ll close this for now; if the rebuild kicks up any issues, we can reopen it. Thanks! Attachment 367913 [details] pushed as a05a21b - GValue – Don't cast G_VALUE_TYPE() argument to GValue*
We did a build, and NetworkManager broke, so unfortunately we have to revert this. :-( commit ca95aa7e12359656cadd712f8c1fc8a3cbac71b7 (HEAD -> master, origin/master, origin/HEAD) Author: Philip Withnall <withnall@endlessm.com> Date: Thu Feb 8 14:28:32 2018 +0000 Revert "GValue – Don't cast G_VALUE_TYPE() argument to GValue*" After building a test run of all GNOME modules against this, we can conclude that it is a visible API break: it breaks the NetworkManager build. http://build.gnome.org/continuous/buildmaster/builds/2018/02/08/46/build/log-NetworkManager.txt NetworkManager is (almost legitimately) passing a gpointer to G_VALUE_TYPE, which I think is a use case we should continue supporting. This reverts commit a05a21bec2f09a7454aea27a0f8aa4739b686878. https://bugzilla.gnome.org/show_bug.cgi?id=793186
Bug #793302 filed with the NetworkManager fix.