Created attachment 155113 [details] [review]
remove unused gst_element_default_error()

Created attachment 155114 [details] [review]
registry: remove unused function

Actually, there was two functions with the same name, but only one was
used.

Created attachment 155115 [details] [review]
fixme: Add -Wno-error to avoid Bison warnings

This needs to be handled properly, but I have no clue how yet

Created attachment 155116 [details] [review]
Fix signedness warnings

... and all the other things to make the code not throw warnings with
-Wextra -Wno-missing-field-initializers -Wno-unused-parameter

Created attachment 155117 [details] [review]
Remove gst_element_request_compatible_pad()

No header declares it.

Created attachment 155118 [details] [review]
bytewriter: Fix float/double write functions to match bytereader

Created attachment 155119 [details] [review]
Fixes for -Wmissing-declarations -Wmissing-prototypes

Does not include fix for bison/flex generated code.

Created attachment 155120 [details] [review]
Make code safe for -Wredundant-decls

Includes a tiny change of the GST_BOILERPLATE_FULL() macro:
The get_type() function is no longer declared before being defined.

Created attachment 155121 [details] [review]
Fixes for -Wold-style-definition

Created attachment 155122 [details] [review]
benchmarks: Remove unneeded g_thread_exit()

Created attachment 155123 [details] [review]
Include gstversion.h for GST_VERSION_NANO

Fixes -Wundef

Created attachment 155124 [details] [review]
Fixes for -Wswitch-default

Created attachment 155126 [details] [review]
Fixes for -Wwrite-strings

This changes some APIs in compatible ways:
- Some functions now take "const char *" arguments, not "char *"
- Some structs now have "conts char *" members, not "char *"
The changes may cause warnings when compiling with the right warning
flags. You've been warned.

Created attachment 155127 [details] [review]
Add all the warning flags we just cleaned up for.

Some comments:

- Patches are against 0.10.26.3
- The testsuite is not fixed yet - make runs without warnings though.

Oh, very important:

I don't have a proper fix for the crappy bison-generated code yet. My idea was to introduce NOERROR_CFLAGS and allow using them for imported or generated code, but I don't have an idea how to do this with forte.
It would look like this:
crappy_files_la_CFLAGS = $(GST_ALL_CFLAGS) $(NOERROR_CFLAGS)

NOERROR_CFLAGS would be set to "-Wno-error".

Note that this would still show the warnings, just not abort compilation.

Review of attachment 155115 [details] [review]:

I don't think we can assume all compilers support -Wno-error

(In reply to comment #17)
> Review of attachment 155115 [details] [review]:
> 
> I don't think we can assume all compilers support -Wno-error
>
Yeah, that's why there's comment 16 and the commit you quoted says "fixme". :)

Comment on attachment 155124 [details] [review]
Fixes for -Wswitch-default

Not a big fan of this. I don't think this is useful in a GStreamer context. In the one or two places where it might be useful (e.g. gstvideo) it should just be added specifically. So -1 from me. This is just a nuisance.

Comment on attachment 155119 [details] [review]
Fixes for -Wmissing-declarations -Wmissing-prototypes

Guess we can live with that. Might even be useful.

But what's with the gst_element_request_compatible_pad() (re-)addition? Why first remove it in an earlier patch? Why not just make it static instead?

Comment on attachment 155117 [details] [review]
Remove gst_element_request_compatible_pad()

Maybe no header declares it, but it's still used further down in gstutils.c as far as I can tell. And seeing that you're re-adding it later as static function in the "Fixes-for--Wmissing-declarations--Wmissing-prototy.patch", maybe you could just make it static here?

Comment on attachment 155113 [details] [review]
remove unused gst_element_default_error()

removing gst_element_default_error() should be fine seeing that it's not used anywhere and not in any header either..

Comment on attachment 155126 [details] [review]
Fixes for -Wwrite-strings

Fixes for -Wwrite-strings: a bit awkward in places, but seems like a good idea to me.

(In reply to comment #20)
> (From update of attachment 155119 [details] [review])
> But what's with the gst_element_request_compatible_pad() (re-)addition? Why
> first remove it in an earlier patch? Why not just make it static instead?
>
It seems I messed that up (forgetting --amend to git commit). I had wanted to merge the two commits and just declare that function static. Will fix that when I commit.

(In reply to comment #19)
> (From update of attachment 155124 [details] [review])
> Not a big fan of this. I don't think this is useful in a GStreamer context. In
> the one or two places where it might be useful (e.g. gstvideo) it should just
> be added specifically. So -1 from me. This is just a nuisance.
>
-Wswitch-default is not a big of a problem usually - it's just "default: break" or "default: assert_not_reached(); break;", but it comes in very handy when you refactor things and add values to enums, because it will actually abort with the assertion while testing and not silently do nothing.
I agree that from a code beauty and maintenance perspective it's not necessary.
I also didn't find a bug while doing this, so it seems to get caught very well - unless you count the fact the GstStateTransition misses GST_STATE_FOO_TO_FOO transitions.

Review of attachment 155116 [details] [review]:

I'm not impressed by all the casting that needs to be done to make signed and unsigned compares work with -Wsigned-compare.  IMO, this makes the code much more prone to errors.

Review of attachment 155119 [details] [review]:

I don't like all the extra "#ifdef GST_DISABLE_DEPRECATED" crap.

Comment on attachment 155124 [details] [review]
Fixes for -Wswitch-default

I happen to like -Wswitch-default, but I don't think an assertion failure is the appropriate response.  It should be g_return_if_fail() or similar.

Review of attachment 155116 [details] [review]:

The common submodule change looks like an accidental git commit -a artefact.

::: gst/gstpad.c
@@ +2996,3 @@
   /* sanity check (only if caps are the same) */
+  if (G_LIKELY (newcaps == caps)
+      && G_UNLIKELY (GST_BUFFER_SIZE (*buf) < (unsigned) size))

May I suggest (unsigned int) or (guint) here instead?

> I'm not impressed by all the casting that needs to be done to make signed and
> unsigned compares work with -Wsigned-compare.  IMO, this makes the code much
> more prone to errors.

I was surprised there wasn't more casting needed actually.

Something I've been wondering: is this guaranteed to work in ANSI C or dependent on compiler whims:

  if ((guint64) ts == -1)
    ...

I was under the impression that you can't rely on compilers not just optimising this away as 'comparison always false due to incompatible range', but have been too lazy to find out for sure so far. If this is the case, then -Wsigned-compare probably makes sense, no? (Even more so if current versions of gcc do what we mean in this case).

(In reply to comment #26)
> Review of attachment 155119 [details] [review]:
> 
> I don't like all the extra "#ifdef GST_DISABLE_DEPRECATED" crap.
>
Yeah, the deprecated handling (in all of GNOME) is rather bad if you want to increase the number of warning flags. I implemented the version that was both sane and required the smallest diff. There's multiple ways to solve this (in order of my preference):

1) Compile GStreamer without GST_DISABLE_DEPRECATED. That's bad because we can't check we don't use deprecated symbols ourselves.

2) in gstfoo.c, do:
#undef GST_DISBALE_DPRECATED
#include "gstfoo.h
#define GST_DISABLE_DEPRECATED
That's similarly bad (it doesn't make sure the file doesn't use the deprecated symbols) and it looks ugly, too.

3) The way in the patch, where I declare the function before implementing it.

4) Move all deprecated functions into gst_deprecated.c and #undef GST_DISABLE_DEPRECATED in there. I didn't do that because it'd have blown up the diffs. Note also that it might not work if the deprecated stuff can't be moved to a different file due to relying on static functions used by non-deprecated stuff.

5) Use G_GNUC_DEPRECATED instead of GST_DISABLE_DEPRECATED. This would also allow deprecating typedefs and public struct members. I don't know how well that method works when handling deprecated code though. And I don't know how well it's integrated into the GNOME tools (like gtk-doc).

(In reply to comment #25)
> Review of attachment 155116 [details] [review]:
> 
> I'm not impressed by all the casting that needs to be done to make signed and
> unsigned compares work with -Wsigned-compare.  IMO, this makes the code much
> more prone to errors.
>
There's in general 2 sources of errors for the required casting:
1) offsets versus differences in offsets (byte or time). It's the same bad thing as size_t vs ssize_t. I'm not aware of a way to properly handle that, but it requires particular care so enabling warnings for this makes sense I think.
We can get around the casts if we use explicit assignments instead when we want to interpret a diff as an absolute position (like I did with real_offset variable in the typefind helper).

2) Bad API design. I think all the places I've found marked this as FIXME 0.11 already, but it's unfortunately not something I can fix today, so I have to do the casts. It's especially bad if it's out variables to function calls.
Another big contributor also is the fact that we mark invalid timestamps or offsets as "(guint64) -1". We have GST_CLOCK_TIME_NONE/GST_CLOCK_TIME_IS_VALID to hide this for timestamps, but offsets only have GST_BUFFER_OFFSET_NONE. A IS_VALID() macro is missing here and the BUFFER_OFFSET_NONE name discourages use to me for anything but buffers.

(In reply to comment #27)
> (From update of attachment 155124 [details] [review])
> I happen to like -Wswitch-default, but I don't think an assertion failure is
> the appropriate response.  It should be g_return_if_fail() or similar.
>
I want something that makes developers very aware that they missed a place when adding new values. And g_assert_not_reached() matches that idea. If GStreamer doesn't disable assertions casuing crashes in releases, I could just change that to g_warn_if_reached() or whatever you think fits.

(In reply to comment #30)
non-deprecated stuff.
> 
> 5) Use G_GNUC_DEPRECATED instead of GST_DISABLE_DEPRECATED. This would also
> allow deprecating typedefs and public struct members. I don't know how well
> that method works when handling deprecated code though. And I don't know how
> well it's integrated into the GNOME tools (like gtk-doc).

gtk-doc does not know about G_GNUC_DEPRECATED.

(In reply to comment #27)
> (From update of attachment 155124 [details] [review])
> I happen to like -Wswitch-default, but I don't think an assertion failure is
> the appropriate response.  It should be g_return_if_fail() or similar.

If its not caused by external inputs we should not use g_return_{val_,}if_fail().

Please revert 57d5db424c68ab5a61f33ce36ce0179eb30251ac . Apparently the other function was used on Windows (now gcc claims that gst_registry_binary_cache_write is declared implicitly).

Comment on attachment 155114 [details] [review]
registry: remove unused function

This one was reverted.

Comment on attachment 155115 [details] [review]
fixme: Add -Wno-error to avoid Bison warnings

This has been solved in a much nicer way.

What are we going to do with this? Wouldn't it make sense to push e.g. the signedness patch?

Let's just close this.

I'm not so in favour of the signedness patch (or the others) - too much hassle for too little gain IMHO, esp. since we have problems with that in GStreamer all over the place not least because of things like the query API.