search

dunst-project / dunst

Lightweight and customizable notification daemon
https://dunst-project.org
Other
4.44k stars 338 forks source link

Fix compiler warnings #1232

Closed zappolowski closed 5 months ago

zappolowski commented 8 months ago

As noted in #1229 the code does not compile cleanly on recent compilers.

At the moment, this builds without warnings on GCC 13.2.1 and clang 16.0.6.

Fixes #1229

fwsmit commented 6 months ago

I'm not sure whether wayland allows for NULL in callbacks and couldn't find any information. So, I decided to just implement all needed handlers as empty methods to get rid of the noop functions.

This is standard practice. I've taken this from mako, but there are also plenty of other places where this is used. So I think that should be fine. It is the most clear way to write it. Does the noop function give warnings?

zappolowski commented 6 months ago

Does the noop function give warnings?

Yes:

src/wayland/foreign_toplevel.c:15:17: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
static void noop() {
                ^
                 void

when compiling with clang 16.0.6. gcc 13.2.1 requires -Wstrict-prototype to warn about this as well.

As the CI builds using both compilers with -Werror, I wanted to have this fixed (better than ignored/silenced).

fwsmit commented 6 months ago

Does the noop function give warnings?

Yes:

src/wayland/foreign_toplevel.c:15:17: error: a function declaration without a prototype is deprecated in all versions of C [-Werror,-Wstrict-prototypes]
static void noop() {
                ^
                 void

when compiling with clang 16.0.6. gcc 13.2.1 requires -Wstrict-prototype to warn about this as well.

As the CI builds using both compilers with -Werror, I wanted to have this fixed (better than ignored/silenced).

I tried reproducing this warning, but I couldn't on gcc 13.2.1. But I have one more idea to maybe still be able to keep the noop function, namely by using variadic arguments. Let me know if that works

static void noop(...) {

}
zappolowski commented 6 months ago

I tried reproducing this warning, but I couldn't on gcc 13.2.1.

I can get an compilation error when using

$ env CFLAGS='-Werror -Wstrict-prototypes' make CC=gcc -B src/wayland/foreign_toplevel.o

on gcc 13.2.1 or

$ env CFLAGS=-Werror make CC=clang -B src/wayland/foreign_toplevel.o

on clang 16.0.6.

But I have one more idea to maybe still be able to keep the noop function, namely by using variadic arguments. Let me know if that works

static void noop(...) {

}

I've already tried that. Two issues with this approach

  1. variadic functions need at least one named parameter before the ellipsis (this is not that bad, as all callbacks use void *user_data as first argument).
  2. the function signatures are still considered incompatible by the compiler: error: incompatible function pointer types initializing 'void (*)(void *, struct zwlr_foreign_toplevel_handle_v1 *, const char *)' with an expression of type 'void (void *, ...)' [-Wincompatible-function-pointer-types] which happens for both compilers.
codecov-commenter commented 6 months ago

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (2fcea84) 65.45% compared to head (fe5ac68) 65.39%.

Files Patch % Lines
src/wayland/wl.c 0.00% 14 Missing :warning:
src/output.c 0.00% 2 Missing :warning:
src/wayland/foreign_toplevel.c 0.00% 2 Missing :warning:
src/settings.c 50.00% 1 Missing :warning:
test/queues.c 0.00% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1232 +/- ## ========================================== - Coverage 65.45% 65.39% -0.07% ========================================== Files 46 46 Lines 7749 7755 +6 ========================================== - Hits 5072 5071 -1 - Misses 2677 2684 +7 ``` | [Flag](https://app.codecov.io/gh/dunst-project/dunst/pull/1232/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/dunst-project/dunst/pull/1232/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project) | `65.39% <23.07%> (-0.07%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dunst-project#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fwsmit commented 6 months ago

I tried reproducing this warning, but I couldn't on gcc 13.2.1.

I can get an compilation error when using

$ env CFLAGS='-Werror -Wstrict-prototypes' make CC=gcc -B src/wayland/foreign_toplevel.o

on gcc 13.2.1 or

$ env CFLAGS=-Werror make CC=clang -B src/wayland/foreign_toplevel.o

on clang 16.0.6.

But I have one more idea to maybe still be able to keep the noop function, namely by using variadic arguments. Let me know if that works

static void noop(...) {

}

I've already tried that. Two issues with this approach

1. variadic functions need at least one named parameter before the ellipsis (this is not that bad, as all callbacks use `void *user_data` as first argument).

2. the function signatures are still considered incompatible by the compiler:
   `error: incompatible function pointer types initializing 'void (*)(void *, struct zwlr_foreign_toplevel_handle_v1 *, const char *)' with an expression of type 'void (void *, ...)' [-Wincompatible-function-pointer-types]`
   which happens for both compilers.

I can indeed reproduce it with the line you gave me. But it seems -Wstrict-prototypes is not included in -Wall. So I'm fine with leaving out this warning, so we can keep our noop functions (to me these are more clear than having empty functions).

zappolowski commented 6 months ago

I can indeed reproduce it with the line you gave me. But it seems -Wstrict-prototypes is not included in -Wall. So I'm fine with leaving out this warning, so we can keep our noop functions (to me these are more clear than having empty functions).

This just works for builds using gcc. clang will still fail as there -Wall also includes -Wstrict-prototypes. One solution (I strongly dislike) is to use -Wall -Wno-strict-prototypes for clang.

fwsmit commented 5 months ago

I can indeed reproduce it with the line you gave me. But it seems -Wstrict-prototypes is not included in -Wall. So I'm fine with leaving out this warning, so we can keep our noop functions (to me these are more clear than having empty functions).

This just works for builds using gcc. clang will still fail as there -Wall also includes -Wstrict-prototypes. One solution (I strongly dislike) is to use -Wall -Wno-strict-prototypes for clang.

Yeah, okay it's better to just fix the warning, since the -Wstrict-prototypes can actually give some good warnings as well.

fwsmit commented 5 months ago

Maybe rename the functions that do nothing with empty_FUNCNAME. This makes it more clear in the listener structs.

zappolowski commented 5 months ago

Maybe rename the functions that do nothing with empty_FUNCNAME. This makes it more clear in the listener structs.

When making use of these callbacks later (I don't know whether this will ever happen), one needs to remember to rename them or they will be misleading. I don't know, whether this is actually useful then.

fwsmit commented 5 months ago

Okay, I've merged it now