After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 774293 - msvc: fix some warnings, disable others
msvc: fix some warnings, disable others
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-build
git master
Other Windows
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-11-11 19:12 UTC by Scott D Phillips
Modified: 2016-11-17 20:58 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH gst-build] Add ignored warnings for MSVC (1.35 KB, patch)
2016-11-11 19:14 UTC, Scott D Phillips
none Details | Review
[PATCH gstreamer 1/2] taglist: remove `return void` in gst_tag_register (1.08 KB, patch)
2016-11-11 19:15 UTC, Scott D Phillips
committed Details | Review
[PATCH gstreamer 2/2] Change some types to match their prototypes (2.29 KB, patch)
2016-11-11 19:15 UTC, Scott D Phillips
none Details | Review
[PATCH gst-plugins-base 1/2] Remove 'return' from `void` functions (1.35 KB, patch)
2016-11-11 19:15 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-base 2/2] Use intermediate guint when handling GstVideoMultiviewFlags (2.57 KB, patch)
2016-11-11 19:16 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-good 1/2] splitfilesrc: update uri_get_type to match the prototype in GstURIHandlerInterface (779 bytes, patch)
2016-11-11 19:16 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-good 2/2] Use intermediate guint when handling GstVideoMultiviewFlags (1.48 KB, patch)
2016-11-11 19:17 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-bad 1/3] mpegtsdemux: fix operator precedence in SAFE_FOURCC_ARGS (1.10 KB, patch)
2016-11-11 19:17 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-bad 2/3] Remove 'return' from `void` functions (826 bytes, patch)
2016-11-11 19:17 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-bad 3/3] winks: undef windows SDK macros before redefining them (953 bytes, patch)
2016-11-11 19:18 UTC, Scott D Phillips
committed Details | Review
[PATCH gstreamer v2 2/2] Change some types to match their prototypes (2.60 KB, patch)
2016-11-14 18:32 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-build v2] Add ignored warnings for MSVC (1.30 KB, patch)
2016-11-15 01:24 UTC, Scott D Phillips
rejected Details | Review
[PATCH gstreamer 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions (3.96 KB, patch)
2016-11-15 01:25 UTC, Scott D Phillips
committed Details | Review
[PATCH gstreamer 4/4] typefindhelper: Update prototype of helper_find_suggest() (1.01 KB, patch)
2016-11-15 01:25 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-base 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions (8.01 KB, patch)
2016-11-15 01:25 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-base 4/4] sdp: cast away const in call to g_free (920 bytes, patch)
2016-11-15 01:26 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-bad 4/5] Cast away const from GstMetaInfo in *_get_meta_info() functions (1.54 KB, patch)
2016-11-15 01:27 UTC, Scott D Phillips
committed Details | Review
[PATCH gst-plugins-bad 5/5] Fix some MSVC warnings about const-ness (1.95 KB, patch)
2016-11-15 01:27 UTC, Scott D Phillips
committed Details | Review

Description Scott D Phillips 2016-11-11 19:12:10 UTC
Add ignored msvc warnings to gst-build's meson.build. Some warnings msvc makes are legitimate-ish, so fix those.
Comment 1 Scott D Phillips 2016-11-11 19:14:45 UTC
Created attachment 339656 [details] [review]
[PATCH gst-build] Add ignored warnings for MSVC
Comment 2 Scott D Phillips 2016-11-11 19:15:14 UTC
Created attachment 339657 [details] [review]
[PATCH gstreamer 1/2] taglist: remove `return void` in gst_tag_register
Comment 3 Scott D Phillips 2016-11-11 19:15:39 UTC
Created attachment 339658 [details] [review]
[PATCH gstreamer 2/2] Change some types to match their prototypes
Comment 4 Scott D Phillips 2016-11-11 19:15:59 UTC
Created attachment 339659 [details] [review]
[PATCH gst-plugins-base 1/2] Remove 'return' from `void` functions
Comment 5 Scott D Phillips 2016-11-11 19:16:18 UTC
Created attachment 339660 [details] [review]
[PATCH gst-plugins-base 2/2] Use intermediate guint when handling GstVideoMultiviewFlags
Comment 6 Scott D Phillips 2016-11-11 19:16:45 UTC
Created attachment 339661 [details] [review]
[PATCH gst-plugins-good 1/2] splitfilesrc: update uri_get_type to match the prototype in GstURIHandlerInterface
Comment 7 Scott D Phillips 2016-11-11 19:17:10 UTC
Created attachment 339662 [details] [review]
[PATCH gst-plugins-good 2/2] Use intermediate guint when handling GstVideoMultiviewFlags
Comment 8 Scott D Phillips 2016-11-11 19:17:35 UTC
Created attachment 339663 [details] [review]
[PATCH gst-plugins-bad 1/3] mpegtsdemux: fix operator precedence in SAFE_FOURCC_ARGS
Comment 9 Scott D Phillips 2016-11-11 19:17:57 UTC
Created attachment 339664 [details] [review]
[PATCH gst-plugins-bad 2/3] Remove 'return' from `void` functions
Comment 10 Scott D Phillips 2016-11-11 19:18:16 UTC
Created attachment 339665 [details] [review]
[PATCH gst-plugins-bad 3/3] winks: undef windows SDK macros before redefining them
Comment 11 Sebastian Dröge (slomo) 2016-11-12 08:47:46 UTC
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 :)
Comment 12 Sebastian Dröge (slomo) 2016-11-12 08:54:25 UTC
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?
Comment 13 Nirbheek Chauhan 2016-11-12 12:38:58 UTC
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.
Comment 14 Sebastian Dröge (slomo) 2016-11-12 14:27:25 UTC
(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.
Comment 15 Scott D Phillips 2016-11-14 16:17:15 UTC
(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.
Comment 16 Sebastian Dröge (slomo) 2016-11-14 16:27:52 UTC
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 :)
Comment 17 Tim-Philipp Müller 2016-11-14 16:34:36 UTC
With gcc g_once_init_{enter,leave} is just a macro and there's a (gpointer) cast which makes the const go away.
Comment 18 Scott D Phillips 2016-11-14 18:32:15 UTC
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 19 Sebastian Dröge (slomo) 2016-11-14 19:04:20 UTC
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
Comment 20 Sebastian Dröge (slomo) 2016-11-14 19:06:18 UTC
(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().
Comment 21 Tim-Philipp Müller 2016-11-14 19:18:06 UTC
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 ;))
Comment 22 Scott D Phillips 2016-11-14 19:24:10 UTC
(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.
Comment 23 Scott D Phillips 2016-11-15 01:23:55 UTC
(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.
Comment 24 Scott D Phillips 2016-11-15 01:24:37 UTC
Created attachment 339858 [details] [review]
[PATCH gst-build v2] Add ignored warnings for MSVC
Comment 25 Scott D Phillips 2016-11-15 01:25:06 UTC
Created attachment 339859 [details] [review]
[PATCH gstreamer 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions
Comment 26 Scott D Phillips 2016-11-15 01:25:27 UTC
Created attachment 339860 [details] [review]
[PATCH gstreamer 4/4] typefindhelper: Update prototype of helper_find_suggest()
Comment 27 Scott D Phillips 2016-11-15 01:25:50 UTC
Created attachment 339861 [details] [review]
[PATCH gst-plugins-base 3/4] Cast away const from GstMetaInfo in *_get_meta_info() functions
Comment 28 Scott D Phillips 2016-11-15 01:26:14 UTC
Created attachment 339862 [details] [review]
[PATCH gst-plugins-base 4/4] sdp: cast away const in call to g_free
Comment 29 Scott D Phillips 2016-11-15 01:27:04 UTC
Created attachment 339863 [details] [review]
[PATCH gst-plugins-bad 4/5] Cast away const from GstMetaInfo in *_get_meta_info() functions
Comment 30 Scott D Phillips 2016-11-15 01:27:24 UTC
Created attachment 339864 [details] [review]
[PATCH gst-plugins-bad 5/5] Fix some MSVC warnings about const-ness
Comment 31 Sebastian Dröge (slomo) 2016-11-15 12:19:36 UTC
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
Comment 32 Sebastian Dröge (slomo) 2016-11-15 12:20:35 UTC
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 33 Sebastian Dröge (slomo) 2016-11-15 12:22:15 UTC
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 34 Sebastian Dröge (slomo) 2016-11-15 12:24:26 UTC
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 35 Sebastian Dröge (slomo) 2016-11-15 12:25:17 UTC
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
Comment 36 Sebastian Dröge (slomo) 2016-11-15 12:27:59 UTC
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
Comment 37 Jan Schmidt 2016-11-15 12:44:23 UTC
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 :)
Comment 38 Jan Schmidt 2016-11-15 12:47:06 UTC
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.
Comment 39 Sebastian Dröge (slomo) 2016-11-15 12:54:38 UTC
Indeed, thanks for the correction
Comment 40 Scott D Phillips 2016-11-17 20:57:30 UTC
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.