GNOME Bugzilla – Bug 774293
msvc: fix some warnings, disable others
Last modified: 2016-11-17 20:58:19 UTC
Add ignored msvc warnings to gst-build's meson.build. Some warnings msvc makes are legitimate-ish, so fix those.
Created attachment 339656 [details] [review] [PATCH gst-build] Add ignored warnings for MSVC
Created attachment 339657 [details] [review] [PATCH gstreamer 1/2] taglist: remove `return void` in gst_tag_register
Created attachment 339658 [details] [review] [PATCH gstreamer 2/2] Change some types to match their prototypes
Created attachment 339659 [details] [review] [PATCH gst-plugins-base 1/2] Remove 'return' from `void` functions
Created attachment 339660 [details] [review] [PATCH gst-plugins-base 2/2] Use intermediate guint when handling GstVideoMultiviewFlags
Created attachment 339661 [details] [review] [PATCH gst-plugins-good 1/2] splitfilesrc: update uri_get_type to match the prototype in GstURIHandlerInterface
Created attachment 339662 [details] [review] [PATCH gst-plugins-good 2/2] Use intermediate guint when handling GstVideoMultiviewFlags
Created attachment 339663 [details] [review] [PATCH gst-plugins-bad 1/3] mpegtsdemux: fix operator precedence in SAFE_FOURCC_ARGS
Created attachment 339664 [details] [review] [PATCH gst-plugins-bad 2/3] Remove 'return' from `void` functions
Created attachment 339665 [details] [review] [PATCH gst-plugins-bad 3/3] winks: undef windows SDK macros before redefining them
Review of attachment 339658 [details] [review]: ::: gst/gstevent.c @@ +673,3 @@ */ GstEvent * +gst_event_new_stream_group_done (const guint group_id) const here makes no sense as argument is passed by value. Please fix the prototype instead :)
Review of attachment 339656 [details] [review]: ::: meson.build @@ +17,3 @@ + add_global_arguments( + '/wd4018', # implicit signed/unsigned conversion + '/wd4090', # implicit const conversion This is non-const to const conversion? Or also the other way around?
Review of attachment 339660 [details] [review]: I think a better way to handle this is to do a cast to guint wherever it is used as a guint.
(In reply to Nirbheek Chauhan from comment #13) > Review of attachment 339660 [details] [review] [review]: > > I think a better way to handle this is to do a cast to guint wherever it is > used as a guint. The problem is that it's used as a pointer. You can safely dereference a guint* casted to SomeEnum*, or the other way around. Conversion of the actual values happens transparently, just like between the various int types.
(In reply to Sebastian Dröge (slomo) from comment #12) > Review of attachment 339656 [details] [review] [review]: > > ::: meson.build > @@ +17,3 @@ > + add_global_arguments( > + '/wd4018', # implicit signed/unsigned conversion > + '/wd4090', # implicit const conversion > > This is non-const to const conversion? Or also the other way around? This is const to non-const. I think it is mostly in code like: > static const Something *a; > if (g_once_init_enter (a)) { > a = ... I'm not sure why gcc doesn't care about losing the const there. If you think it better to keep those warnings on I can take a look into it what changes would be required.
gcc should also warn about that if there is no explicit cast that makes the const go away. I'd rather have these warnings fixed :)
With gcc g_once_init_{enter,leave} is just a macro and there's a (gpointer) cast which makes the const go away.
Created attachment 339826 [details] [review] [PATCH gstreamer v2 2/2] Change some types to match their prototypes change prototype of gst_event_new_stream_group_done, not definition
Comment on attachment 339826 [details] [review] [PATCH gstreamer v2 2/2] Change some types to match their prototypes commit 5a72c23a54ffc8d61ab94969844e73b3b1a293ec Author: Scott D Phillips <scott.d.phillips@intel.com> Date: Fri Nov 11 10:30:44 2016 -0800 Change some types to match their prototypes Particularly note that the underlying integer type of the enum GstTypeFindProbability is implementation dependent and may not match guint. https://bugzilla.gnome.org/show_bug.cgi?id=774293
(In reply to Tim-Philipp Müller from comment #17) > With gcc g_once_init_{enter,leave} is just a macro and there's a (gpointer) > cast which makes the const go away. Indeed. Too bad there are no casts that preserve the const'ness of the argument. Having the pointer passed there const seems like a bug though, the value is not const but written to by g_once_init_leave().
Funny how we ping-pong this around every now and then ;) commit 461178fb59d3ee23412a089083e45100fe1e4e2c Author: Brian Cameron <brian.cameron at oracle.com> Date: Thu May 24 11:02:53 2012 +0200 typefind: fix prototype of helper_find_suggest The proto for helper_find_suggest has a different argument than the actual function in the same file has. This causes the Sun Studio compiler to fail. Fixes https://bugzilla.gnome.org/show_bug.cgi?id=676624 (Not that we care that much about Sun Studio ;))
(In reply to Tim-Philipp Müller from comment #21) > Funny how we ping-pong this around every now and then ;) > > commit 461178fb59d3ee23412a089083e45100fe1e4e2c > Author: Brian Cameron <brian.cameron at oracle.com> > Date: Thu May 24 11:02:53 2012 +0200 > > typefind: fix prototype of helper_find_suggest > > The proto for helper_find_suggest has a different argument than the > actual > function in the same file has. This causes the Sun Studio compiler to > fail. > > Fixes https://bugzilla.gnome.org/show_bug.cgi?id=676624 > > (Not that we care that much about Sun Studio ;)) uh good point. not sure why msvc isn't barfing on that. I'll fix the proto too and maybe run everything through gcc as well to make sure i've shaken down to a steady state on warnings.
(In reply to Sebastian Dröge (slomo) from comment #20) > (In reply to Tim-Philipp Müller from comment #17) > > With gcc g_once_init_{enter,leave} is just a macro and there's a (gpointer) > > cast which makes the const go away. > > Indeed. Too bad there are no casts that preserve the const'ness of the > argument. > > Having the pointer passed there const seems like a bug though, the value is > not const but written to by g_once_init_leave(). Digging into this more, it seems there's actually not a cast in gthread.h but just some static assert type stuff about what is passed in. What seems to be going on is that there are different sets of rules for qualified pointer conversion in C and C++. The places that MSVC warns about seem to be OK by the C++ rules but not by the C rules. MSVC has the separate rule sets the two languages whereas gcc and clang just use the C++ rules for both languages. This is just an armchair linguist's conclusion though, so caveat lector.
Created attachment 339858 [details] [review] [PATCH gst-build v2] Add ignored warnings for MSVC
Created attachment 339859 [details] [review] [PATCH gstreamer 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions
Created attachment 339860 [details] [review] [PATCH gstreamer 4/4] typefindhelper: Update prototype of helper_find_suggest()
Created attachment 339861 [details] [review] [PATCH gst-plugins-base 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions
Created attachment 339862 [details] [review] [PATCH gst-plugins-base 4/4] sdp: cast away const in call to g_free
Created attachment 339863 [details] [review] [PATCH gst-plugins-bad 4/5] Cast away const from GstMetaInfo in *_get_meta_info() functions
Created attachment 339864 [details] [review] [PATCH gst-plugins-bad 5/5] Fix some MSVC warnings about const-ness
Comment on attachment 339858 [details] [review] [PATCH gst-build v2] Add ignored warnings for MSVC Looks good to me, I'll keep this for someone else to merge though
Review of attachment 339859 [details] [review]: ::: gst/gstbuffer.c @@ +2482,3 @@ static const GstMetaInfo *meta_info = NULL; + if (g_once_init_enter ((GstMetaInfo **) & meta_info)) { I don't think that is correct. The const at the variable declaration should go away. It is *not* const, otherwise it would always stay NULL.
Comment on attachment 339861 [details] [review] [PATCH gst-plugins-base 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions Same as above
Comment on attachment 339862 [details] [review] [PATCH gst-plugins-base 4/4] sdp: cast away const in call to g_free This should be non-const in the declaration of the variable, and casted to a const gchar ** for gst_sdp_message_add_time(). The declaration is wrong, it's not used const (i.e. it is not always NULL as during initialization).
Comment on attachment 339863 [details] [review] [PATCH gst-plugins-bad 4/5] Cast away const from GstMetaInfo in *_get_meta_info() functions Same as above
Review of attachment 339864 [details] [review]: ::: gst-libs/gst/codecparsers/gstjpegparser.c @@ +577,3 @@ for (i = 0; i < num_entries; i++) sorted_entries[i] = &entries[i]; + qsort ((void *) sorted_entries, num_entries, sizeof (sorted_entries[0]), The variable declaration is wrong, this thing is apparently not const as we modify it ::: gst/id3tag/id3tag.c @@ -460,3 @@ } - g_free (strings); variable declaration is wrong, see above
Review of attachment 339864 [details] [review]: ::: gst-libs/gst/codecparsers/gstjpegparser.c @@ +577,3 @@ for (i = 0; i < num_entries; i++) sorted_entries[i] = &entries[i]; + qsort ((void *) sorted_entries, num_entries, sizeof (sorted_entries[0]), No, the variable declaration is correct. const GstJpegHuffmanTableEntry *sorted_entries[256] is a writable non-const array of pointers to const GstJpegHuffmanTableEntry. ::: gst/id3tag/id3tag.c @@ -460,3 @@ } - g_free (strings); No, it's not :)
Review of attachment 339859 [details] [review]: ::: gst/gstbuffer.c @@ +2482,3 @@ static const GstMetaInfo *meta_info = NULL; + if (g_once_init_enter ((GstMetaInfo **) & meta_info)) { You've confused a pointer to a const and a const pointer. It's fine to modify a pointer to a const thing.
Indeed, thanks for the correction
Review of attachment 339858 [details] [review]: Meson 0.36.0 is out now which supports add_project_arguments(), so we will use that per project instead of doing this globally in gst-build.