Bug 98191 - Fix -Wswitch-enum, -Wswitch-default warnings
Summary: Fix -Wswitch-enum, -Wswitch-default warnings
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on: 88922 97357
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-10 14:36 UTC by Simon McVittie
Modified: 2017-04-07 13:03 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
_dbus_validity_to_error_message: add missing cases (1.33 KB, patch)
2016-10-13 22:22 UTC, Simon McVittie
Details | Splinter Review
Move defaults for some switches into a default case (2.98 KB, patch)
2016-10-13 22:23 UTC, Simon McVittie
Details | Splinter Review
Trivial config parser: enumerate the elements we don't care about (2.83 KB, patch)
2016-10-13 22:23 UTC, Simon McVittie
Details | Splinter Review
Stringify DBUS_AUTH_STATE_INVALID (855 bytes, patch)
2016-10-13 22:24 UTC, Simon McVittie
Details | Splinter Review
Marshalling tests: make integer generation more concise (2.34 KB, patch)
2016-10-13 22:24 UTC, Simon McVittie
Details | Splinter Review
Bus driver: add default BusDriverFound switch cases (1.27 KB, patch)
2016-10-13 22:24 UTC, Simon McVittie
Details | Splinter Review
WiP: remaining -Wswitch-* fixes from Thomas Zimmermann (11.96 KB, patch)
2017-01-17 20:10 UTC, Simon McVittie
Details | Splinter Review
[1] bus dispatch tests: treat impossible message_kind as GOT_SOMETHING_ELSE (1.91 KB, patch)
2017-03-24 19:42 UTC, Simon McVittie
Details | Splinter Review
[2] dbus-daemon: silence -Wswitch-default (953 bytes, patch)
2017-03-24 19:42 UTC, Simon McVittie
Details | Splinter Review
[3] _dbus_lm_strerror: move default behaviour inside switch (1.65 KB, patch)
2017-03-24 19:42 UTC, Simon McVittie
Details | Splinter Review
[4] dbus-spawn: assert impossible returns from read functions don't happen (1.44 KB, patch)
2017-03-24 19:43 UTC, Simon McVittie
Details | Splinter Review
[5] test, tools: assert impossible values of local enums are not reached (5.27 KB, patch)
2017-03-24 19:43 UTC, Simon McVittie
Details | Splinter Review
[6] dbus-monitor: handle default case for binary mode header (1.01 KB, patch)
2017-03-24 19:43 UTC, Simon McVittie
Details | Splinter Review
[7] dbus-launch: clarify signal handler (2.03 KB, patch)
2017-03-24 19:43 UTC, Simon McVittie
Details | Splinter Review
[8] DBusTransport: assert that invalid results don't happen (1.45 KB, patch)
2017-03-24 19:44 UTC, Simon McVittie
Details | Splinter Review
[9] DBusTransport: be explicit about _dbus_auth_do_work() results (1.17 KB, patch)
2017-03-24 19:45 UTC, Simon McVittie
Details | Splinter Review
[10] _dbus_global_lock: move success case up into switch (878 bytes, patch)
2017-03-24 19:45 UTC, Simon McVittie
Details | Splinter Review
[11] sysdeps: assert that log severity is one we expect (1.43 KB, patch)
2017-03-24 19:45 UTC, Simon McVittie
Details | Splinter Review
[12] config-parser: treat impossible policy type as IGNORED (845 bytes, patch)
2017-03-24 19:46 UTC, Simon McVittie
Details | Splinter Review
[13] config-parser: assert elements are of a known type (1.06 KB, patch)
2017-03-24 19:46 UTC, Simon McVittie
Details | Splinter Review
[14] config-parser tests: explicitly skip non-comparable elements (1.53 KB, patch)
2017-03-24 19:47 UTC, Simon McVittie
Details | Splinter Review
[15] bus policy: assert that no invalid rule types are seen (1.68 KB, patch)
2017-03-24 19:47 UTC, Simon McVittie
Details | Splinter Review
[16] Stop opting out of -Wswitch-enum and -Wswitch-default (1.08 KB, patch)
2017-03-24 19:48 UTC, Simon McVittie
Details | Splinter Review
[5] test, tools: assert impossible values of local enums are not reached (5.76 KB, patch)
2017-03-24 20:28 UTC, Simon McVittie
Details | Splinter Review
DBusTransport: be explicit about _dbus_auth_do_work() results (1.53 KB, patch)
2017-04-07 11:51 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Simon McVittie 2016-10-10 14:36:51 UTC
+++ This bug was initially created as a clone of Bug #97357 +++

The patches on Bug #97357 include some for -Wswitch-enum and -Wswitch-default (Attachment #127058 [details], Attachment #127059 [details]).

To make this more reviewable, I would prefer to fix these two warnings as a unit, but with each commit doing one thing, by following this process:

* Land what's on Bug #97357, or use my "warnings" branch on Github as a base
  <https://github.com/smcv/dbus/tree/warnings>

* Enable the warnings locally but do not make them fatal, by building with
  make CFLAGS="-Wswitch-enum -Wswitch-default -Wno-error=switch-enum -Wno-error=switch-default"

* Go through each affected switch or closely related group of switches
  (for example switching over values of the same enum can probably be
  grouped), fix both warnings at the same time, and do one commit
  per group

* After all those commits, add one commit removing -Wno-switch-enum and
  -Wno-switch-default from the AX_COMPILER_FLAGS* arguments so that this
  doesn't regress in future

I hope that makes sense.
Comment 1 Simon McVittie 2016-10-13 22:22:43 UTC
Created attachment 127281 [details] [review]
_dbus_validity_to_error_message: add missing cases

From: Thomas Zimmermann <tdz@users.sourceforge.net>

[smcv: split out from a larger commit]
Reviewed-by: Simon McVittie <smcv@debian.org>
Comment 2 Simon McVittie 2016-10-13 22:23:06 UTC
Created attachment 127282 [details] [review]
Move defaults for some switches into a default case

This is clearly equivalent, and quiets -Wswitch-default.

Based on part of a patch by Thomas Zimmermann.
Comment 3 Simon McVittie 2016-10-13 22:23:31 UTC
Created attachment 127283 [details] [review]
Trivial config parser: enumerate the elements we don't  care about

From: Thomas Zimmermann <tdz@users.sourceforge.net>

This quiets -Wswitch-enum warnings. The trivial config parser
is used by the setuid activation helper, and only handles the
elements whose contents influence the operation of that helper:
system service directories, the setuid activation helper itself,
the bus uid, and the bus type.

[smcv: split out from a larger commit; add justification; move
ELEMENT_SERVICEDIR start handler to a functionally equivalent list
of elements whose content we are going to process later]
Reviewed-by: Simon McVittie <smcv@debian.org>
Comment 4 Simon McVittie 2016-10-13 22:24:00 UTC
Created attachment 127284 [details] [review]
Stringify DBUS_AUTH_STATE_INVALID

From: Thomas Zimmermann <tdz@users.sourceforge.net>

[smcv: split out from a larger commit]
Reviewed-by: Simon McVittie <smcv@debian.org>
Comment 5 Simon McVittie 2016-10-13 22:24:24 UTC
Created attachment 127285 [details] [review]
Marshalling tests: make integer generation more concise

From: Thomas Zimmermann <tdz@users.sourceforge.net>

This also avoids -Wswitch-default warnings.

[smcv: split out from a larger commit]
Reviewed-by: Simon McVittie <smcv@debian.org>
Comment 6 Simon McVittie 2016-10-13 22:24:45 UTC
Created attachment 127286 [details] [review]
Bus driver: add default BusDriverFound switch cases

If we get an impossible result, treat it as BUS_DRIVER_FOUND_ERROR.
Comment 7 Simon McVittie 2016-10-13 22:28:23 UTC
https://github.com/smcv/dbus/tree/switch has my work-in-progress on breaking up and reviewing the original attachments: the reviewed bits attached here, my new commits also attached here, and a large commit for "the rest".
Comment 8 Simon McVittie 2017-01-17 20:10:30 UTC
Created attachment 129005 [details] [review]
WiP: remaining -Wswitch-* fixes from Thomas Zimmermann

These need reviewing, and in some cases clarifying.

---

I pushed previous attachments to master for 1.11.10. Here is what's left to break up and review.
Comment 9 Philip Withnall 2017-01-18 10:39:57 UTC
Comment on attachment 129005 [details] [review]
WiP: remaining -Wswitch-* fixes from Thomas Zimmermann

Review of attachment 129005 [details] [review]:
-----------------------------------------------------------------

Looks to me like a lot of the `default` cases should call `_dbus_assert_not_reached()`, since they should never be reached at runtime.

::: bus/dispatch.c
@@ +3556,5 @@
>            _dbus_warn ("Unexpected message after auto activation");
>            goto out;
> +
> +        default:
> +          break;

Would this one be better off having a warning (i.e. same as the GOT_SOMETHING_ELSE case)?

@@ +4252,5 @@
>            _dbus_warn ("Unexpected message after auto activation");
>            goto out;
> +
> +        default:
> +          break;

Would this one be better off having a warning (i.e. same as the GOT_SOMETHING_ELSE case)?
Comment 10 Simon McVittie 2017-01-18 12:20:52 UTC
(In reply to Philip Withnall from comment #9)
> Looks to me like a lot of the `default` cases should call
> `_dbus_assert_not_reached()`, since they should never be reached at runtime.

Yes. I'll attach more patches soon.
Comment 11 Simon McVittie 2017-03-24 19:42:01 UTC
Created attachment 130424 [details] [review]
[1] bus dispatch tests: treat impossible message_kind as  GOT_SOMETHING_ELSE

check_got_service_info() can't actually return an invalid
GotServiceInfo, but if it somehow does, we want to fail the test.
GOT_SOMETHING_ELSE already has that effect, and a similar meaning.

Based on a patch from Thomas Zimmermann.
Comment 12 Simon McVittie 2017-03-24 19:42:18 UTC
Created attachment 130425 [details] [review]
[2] dbus-daemon: silence -Wswitch-default

There should be no way signal_handler() can be called for a signal
we didn't ask for. If it somehow happens, ignore it.

Based on a patch from Thomas Zimmermann.
Comment 13 Simon McVittie 2017-03-24 19:42:39 UTC
Created attachment 130426 [details] [review]
[3] _dbus_lm_strerror: move default behaviour inside switch

This silences -Wswitch-default.

Part of a larger commit from Thomas Zimmermann.
Comment 14 Simon McVittie 2017-03-24 19:43:01 UTC
Created attachment 130427 [details] [review]
[4] dbus-spawn: assert impossible returns from read  functions don't happen

This silences -Wswitch-default.

Based on a patch from Thomas Zimmermann.
Comment 15 Simon McVittie 2017-03-24 19:43:17 UTC
Created attachment 130428 [details] [review]
[5] test, tools: assert impossible values of local enums  are not reached

Based on part of a patch from Thomas Zimmermann.
Comment 16 Simon McVittie 2017-03-24 19:43:35 UTC
Created attachment 130429 [details] [review]
[6] dbus-monitor: handle default case for binary mode  header

Also comment why it's OK to not do anything for the modes that don't
have a header. We are effectively treating the default case as one
of those, on the assumption that future modes are more likely to
lack a header than to have one.

Based on part of a patch from Thomas Zimmermann.
Comment 17 Simon McVittie 2017-03-24 19:43:55 UTC
Created attachment 130430 [details] [review]
[7] dbus-launch: clarify signal handler

We only register signal_handler() for the three signals that we want
to handle as "kill dbus-daemon and exit", so there's no point in the
switch. Silence -Wswitch-default by removing it altogether.

The variable name got_fatal_signal and the verbose message are both
misleading, because actually this is a handler for multiple signals,
not just SIGHUP. Rename them to be generic.

Based on part of a patch from Thomas Zimmermann.
Comment 18 Simon McVittie 2017-03-24 19:44:17 UTC
Created attachment 130431 [details] [review]
[8] DBusTransport: assert that invalid results don't happen

This silences -Wswitch-default.

Based on part of a patch from Thomas Zimmermann.
Comment 19 Simon McVittie 2017-03-24 19:45:14 UTC
Created attachment 130432 [details] [review]
[9] DBusTransport: be explicit about _dbus_auth_do_work()  results

This silences -Wswitch-enum.

Based on part of a patch from Thomas Zimmermann.
Comment 20 Simon McVittie 2017-03-24 19:45:34 UTC
Created attachment 130433 [details] [review]
[10] _dbus_global_lock: move success case up into switch

This silences -Wswitch-default.

Based on part of a patch from Thomas Zimmermann.
Comment 21 Simon McVittie 2017-03-24 19:45:54 UTC
Created attachment 130434 [details] [review]
[11] sysdeps: assert that log severity is one we expect

This silences -Wswitch-default.

Based on part of a patch from Thomas Zimmermann.
Comment 22 Simon McVittie 2017-03-24 19:46:28 UTC
Created attachment 130435 [details] [review]
[12] config-parser: treat impossible policy type as IGNORED

This silences -Wswitch-default.

Based on part of a patch from Thomas Zimmermann.
Comment 23 Simon McVittie 2017-03-24 19:46:47 UTC
Created attachment 130436 [details] [review]
[13] config-parser: assert elements are of a known type

This silences -Wswitch-default.

Based on part of a patch from Thomas Zimmermann.
Comment 24 Simon McVittie 2017-03-24 19:47:23 UTC
Created attachment 130437 [details] [review]
[14] config-parser tests: explicitly skip non-comparable  elements

For these types, the tagged union in the Element struct does not store
anything we could usefuly compare.

Based on part of a patch from Thomas Zimmermann.
Comment 25 Simon McVittie 2017-03-24 19:47:41 UTC
Created attachment 130438 [details] [review]
[15] bus policy: assert that no invalid rule types are seen

This silences -Wswitch-default.

Based on part of a patch from Thomas Zimmermann.
Comment 26 Simon McVittie 2017-03-24 19:48:00 UTC
Created attachment 130439 [details] [review]
[16] Stop opting out of -Wswitch-enum and -Wswitch-default
Comment 27 Simon McVittie 2017-03-24 20:28:38 UTC
Created attachment 130442 [details] [review]
[5] test, tools: assert impossible values of local enums  are not reached

Based on part of a patch from Thomas Zimmermann.

---

Fix another instance of -Wswitch-enum when building for mingw with tests enabled.
Comment 28 Philip Withnall 2017-03-27 11:57:38 UTC
Comment on attachment 130424 [details] [review]
[1] bus dispatch tests: treat impossible message_kind as  GOT_SOMETHING_ELSE

Review of attachment 130424 [details] [review]:
-----------------------------------------------------------------

++
Comment 29 Philip Withnall 2017-03-27 11:58:05 UTC
Comment on attachment 130425 [details] [review]
[2] dbus-daemon: silence -Wswitch-default

Review of attachment 130425 [details] [review]:
-----------------------------------------------------------------

++
Comment 30 Philip Withnall 2017-03-27 11:58:36 UTC
Comment on attachment 130426 [details] [review]
[3] _dbus_lm_strerror: move default behaviour inside switch

Review of attachment 130426 [details] [review]:
-----------------------------------------------------------------

++
Comment 31 Philip Withnall 2017-03-27 11:58:58 UTC
Comment on attachment 130427 [details] [review]
[4] dbus-spawn: assert impossible returns from read  functions don't happen

Review of attachment 130427 [details] [review]:
-----------------------------------------------------------------

++
Comment 32 Philip Withnall 2017-03-27 12:01:59 UTC
Comment on attachment 130428 [details] [review]
[5] test, tools: assert impossible values of local enums  are not reached

Review of attachment 130428 [details] [review]:
-----------------------------------------------------------------

++
Comment 33 Philip Withnall 2017-03-27 12:02:22 UTC
Comment on attachment 130429 [details] [review]
[6] dbus-monitor: handle default case for binary mode  header

Review of attachment 130429 [details] [review]:
-----------------------------------------------------------------

++
Comment 34 Philip Withnall 2017-03-27 12:03:20 UTC
Comment on attachment 130430 [details] [review]
[7] dbus-launch: clarify signal handler

Review of attachment 130430 [details] [review]:
-----------------------------------------------------------------

++
Comment 35 Philip Withnall 2017-03-27 12:03:38 UTC
Comment on attachment 130431 [details] [review]
[8] DBusTransport: assert that invalid results don't happen

Review of attachment 130431 [details] [review]:
-----------------------------------------------------------------

++
Comment 36 Philip Withnall 2017-03-27 12:04:13 UTC
Comment on attachment 130432 [details] [review]
[9] DBusTransport: be explicit about _dbus_auth_do_work()  results

Review of attachment 130432 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-transport.c
@@ +752,5 @@
> +            case DBUS_AUTH_STATE_WAITING_FOR_INPUT:
> +            case DBUS_AUTH_STATE_WAITING_FOR_MEMORY:
> +            case DBUS_AUTH_STATE_HAVE_BYTES_TO_SEND:
> +            case DBUS_AUTH_STATE_NEED_DISCONNECT:
> +            case DBUS_AUTH_STATE_INVALID:

Do we not want to handle DBUS_AUTH_STATE_INVALID separately?
Comment 37 Philip Withnall 2017-03-27 12:04:27 UTC
Comment on attachment 130433 [details] [review]
[10] _dbus_global_lock: move success case up into switch

Review of attachment 130433 [details] [review]:
-----------------------------------------------------------------

++
Comment 38 Philip Withnall 2017-03-27 12:04:54 UTC
Comment on attachment 130434 [details] [review]
[11] sysdeps: assert that log severity is one we expect

Review of attachment 130434 [details] [review]:
-----------------------------------------------------------------

++
Comment 39 Philip Withnall 2017-03-27 12:05:13 UTC
Comment on attachment 130435 [details] [review]
[12] config-parser: treat impossible policy type as IGNORED

Review of attachment 130435 [details] [review]:
-----------------------------------------------------------------

++
Comment 40 Philip Withnall 2017-03-27 12:05:29 UTC
Comment on attachment 130436 [details] [review]
[13] config-parser: assert elements are of a known type

Review of attachment 130436 [details] [review]:
-----------------------------------------------------------------

++
Comment 41 Philip Withnall 2017-03-27 12:05:54 UTC
Comment on attachment 130437 [details] [review]
[14] config-parser tests: explicitly skip non-comparable  elements

Review of attachment 130437 [details] [review]:
-----------------------------------------------------------------

++
Comment 42 Philip Withnall 2017-03-27 12:06:23 UTC
Comment on attachment 130438 [details] [review]
[15] bus policy: assert that no invalid rule types are seen

Review of attachment 130438 [details] [review]:
-----------------------------------------------------------------

++
Comment 43 Philip Withnall 2017-03-27 12:07:13 UTC
Comment on attachment 130439 [details] [review]
[16] Stop opting out of -Wswitch-enum and -Wswitch-default

Review of attachment 130439 [details] [review]:
-----------------------------------------------------------------

yay, ++
Comment 44 Philip Withnall 2017-03-27 12:07:48 UTC
Comment on attachment 130442 [details] [review]
[5] test, tools: assert impossible values of local enums  are not reached

Review of attachment 130442 [details] [review]:
-----------------------------------------------------------------

++
Comment 45 Simon McVittie 2017-04-07 11:51:21 UTC
Created attachment 130743 [details] [review]
DBusTransport: be explicit about _dbus_auth_do_work()  results

Replacement for Attachment #130432 [details], addressing Philip's review comments.

I applied the rest, except for Attachment #130439 [details] which needs to be last.
Comment 46 Philip Withnall 2017-04-07 12:23:44 UTC
Comment on attachment 130743 [details] [review]
DBusTransport: be explicit about _dbus_auth_do_work()  results

Review of attachment 130743 [details] [review]:
-----------------------------------------------------------------

r+
Comment 47 Simon McVittie 2017-04-07 13:03:46 UTC
Thanks, fixed in git for 1.11.12


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.