[PATCH v2] wireless: mark expected switch fall-throughs

[PATCH v2] wireless: mark expected switch fall-throughs

[Gustavo A. R. Silva]

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Warning level 3 was used: -Wimplicit-fallthrough=3

This code was not tested and GCC 7.2.0 was used to compile it.

Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
Changes in v2:
 - Explicity mention in the commit log that this the code was not
   tested.
 - Use warning level 3 instead of 2.
 - Change the form "else: fall through" to "fall through" instead.
 - Address -Wimplicit-fallthrough warnings in net/wireless/scan.c

 net/wireless/chan.c        | 2 ++
 net/wireless/nl80211.c     | 9 +++++++++
 net/wireless/scan.c        | 2 +-
 net/wireless/wext-compat.c | 2 ++
 4 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/wireless/chan.c b/net/wireless/chan.c
index 2db713d..0a45265 100644
--- a/net/wireless/chan.c
+++ b/net/wireless/chan.c
@@ -747,6 +747,7 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
 	case NL80211_CHAN_WIDTH_20:
 		if (!ht_cap->ht_supported)
 			return false;
+		/* fall through */
 	case NL80211_CHAN_WIDTH_20_NOHT:
 		prohibited_flags |= IEEE80211_CHAN_NO_20MHZ;
 		width = 20;
@@ -769,6 +770,7 @@ bool cfg80211_chandef_usable(struct wiphy *wiphy,
 		cap = vht_cap->cap & IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_MASK;
 		if (cap != IEEE80211_VHT_CAP_SUPP_CHAN_WIDTH_160_80PLUS80MHZ)
 			return false;
+		/* fall through */
 	case NL80211_CHAN_WIDTH_80:
 		if (!vht_cap->vht_supported)
 			return false;
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 744b585..0276370 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -1706,6 +1706,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		state->split_start++;
 		if (state->split)
 			break;
+		/* fall through */
 	case 1:
 		if (nla_put(msg, NL80211_ATTR_CIPHER_SUITES,
 			    sizeof(u32) * rdev->wiphy.n_cipher_suites,
@@ -1752,6 +1753,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		state->split_start++;
 		if (state->split)
 			break;
+		/* fall through */
 	case 2:
 		if (nl80211_put_iftypes(msg, NL80211_ATTR_SUPPORTED_IFTYPES,
 					rdev->wiphy.interface_modes))
@@ -1759,6 +1761,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		state->split_start++;
 		if (state->split)
 			break;
+		/* fall through */
 	case 3:
 		nl_bands = nla_nest_start(msg, NL80211_ATTR_WIPHY_BANDS);
 		if (!nl_bands)
@@ -1784,6 +1787,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 				state->chan_start++;
 				if (state->split)
 					break;
+				/* fall through */
 			default:
 				/* add frequencies */
 				nl_freqs = nla_nest_start(
@@ -1837,6 +1841,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 			state->split_start++;
 		if (state->split)
 			break;
+		/* fall through */
 	case 4:
 		nl_cmds = nla_nest_start(msg, NL80211_ATTR_SUPPORTED_COMMANDS);
 		if (!nl_cmds)
@@ -1863,6 +1868,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		state->split_start++;
 		if (state->split)
 			break;
+		/* fall through */
 	case 5:
 		if (rdev->ops->remain_on_channel &&
 		    (rdev->wiphy.flags & WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL) &&
@@ -1880,6 +1886,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		state->split_start++;
 		if (state->split)
 			break;
+		/* fall through */
 	case 6:
 #ifdef CONFIG_PM
 		if (nl80211_send_wowlan(msg, rdev, state->split))
@@ -1890,6 +1897,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 #else
 		state->split_start++;
 #endif
+		/* fall through */
 	case 7:
 		if (nl80211_put_iftypes(msg, NL80211_ATTR_SOFTWARE_IFTYPES,
 					rdev->wiphy.software_iftypes))
@@ -1902,6 +1910,7 @@ static int nl80211_send_wiphy(struct cfg80211_registered_device *rdev,
 		state->split_start++;
 		if (state->split)
 			break;
+		/* fall through */
 	case 8:
 		if ((rdev->wiphy.flags & WIPHY_FLAG_HAVE_AP_SME) &&
 		    nla_put_u32(msg, NL80211_ATTR_DEVICE_AP_SME,
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index d0e7472..0f0b519 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1183,7 +1183,7 @@ cfg80211_inform_bss_data(struct wiphy *wiphy,
 	switch (ftype) {
 	case CFG80211_BSS_FTYPE_BEACON:
 		ies->from_beacon = true;
-		/* fall through to assign */
+		/* fall through - to assign */
 	case CFG80211_BSS_FTYPE_UNKNOWN:
 		rcu_assign_pointer(tmp.pub.beacon_ies, ies);
 		break;
diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
index 06943d9..d522787 100644
--- a/net/wireless/wext-compat.c
+++ b/net/wireless/wext-compat.c
@@ -1337,6 +1337,7 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev)
 			wstats.qual.qual = sig + 110;
 			break;
 		}
+		/* fall through */
 	case CFG80211_SIGNAL_TYPE_UNSPEC:
 		if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_SIGNAL)) {
 			wstats.qual.updated |= IW_QUAL_LEVEL_UPDATED;
@@ -1345,6 +1346,7 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev)
 			wstats.qual.qual = sinfo.signal;
 			break;
 		}
+		/* fall through */
 	default:
 		wstats.qual.updated |= IW_QUAL_LEVEL_INVALID;
 		wstats.qual.updated |= IW_QUAL_QUAL_INVALID;
-- 
2.7.4

Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Johannes Berg]

On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
> 
> Warning level 3 was used: -Wimplicit-fallthrough=3
> 
> This code was not tested and GCC 7.2.0 was used to compile it.

Look, I'm not going to make this any clearer: I'm not applying patches
like that where you've invested no effort whatsoever on verifying that
they're correct.

johannes

Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Gustavo A. R. Silva]

On 10/23/18 9:01 AM, Johannes Berg wrote:
> On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>>
>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>
>> This code was not tested and GCC 7.2.0 was used to compile it.
> 
> Look, I'm not going to make this any clearer: I'm not applying patches
> like that where you've invested no effort whatsoever on verifying that
> they're correct.
> 

How do you suggest me to verify that every part is correct in this type
of patches?

Thanks
--
Gustavo

Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Gustavo A. R. Silva]

On 10/23/18 9:01 AM, Johannes Berg wrote:
> On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>> where we are expecting to fall through.
>>
>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>
>> This code was not tested and GCC 7.2.0 was used to compile it.
> 
> Look, I'm not going to make this any clearer: I'm not applying patches
> like that where you've invested no effort whatsoever on verifying that
> they're correct.
> 

How do you suggest me to verify that every part is correct in this type
of patches?

Thanks

Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Gustavo A. R. Silva]

On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote:
> 
> On 10/23/18 9:01 AM, Johannes Berg wrote:
>> On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>> where we are expecting to fall through.
>>>
>>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>>
>>> This code was not tested and GCC 7.2.0 was used to compile it.
>>
>> Look, I'm not going to make this any clearer: I'm not applying patches
>> like that where you've invested no effort whatsoever on verifying that
>> they're correct.
>>
> 
> How do you suggest me to verify that every part is correct in this type
> of patches?
> 

BTW... I'm under the impression you think that I don't even look at
the code. Is that correct?

I've been working on this for quite a while, and in every case I try to
understand the code in terms of the context in which every warning is
reported.

I look for dependencies between variables in adjacent switch cases, that could
make think it might be OK for some of those cases to fall through to the one
below. I check the names of the case labels to see if there might be any relation
between them, or if they are totally diferent as it might be the case for labels
that indicate the transmision(FOO_TX) or reception of data(FOO_RX). Something
similar to the latter is the case with on/off logic (FOO_ON/FOO_OFF). These are
some of the things I review in the code, so I can have an idea if the warning is
a false positive or an actual bug.

Here is a bug I found yesterday at drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c

5690         case WLAN_CIPHER_SUITE_CCMP:
5691                 key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
5692                 break;
5693         case WLAN_CIPHER_SUITE_TKIP:
5694                 key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
5695         default:
5696                 return -EOPNOTSUPP;
5697         }

Notice how the absence of a break statement is very suspicious in case
WLAN_CIPHER_SUITE_TKIP, because the code for case WLAN_CIPHER_SUITE_CCMP
is pretty similar and in that case there is a break at the bottom. Now,
that's not the only thing that looks supicious: in the absence of a break,
the code falls through to the default case, which returns the error value
-EOPNOTSUPP. So, even when key->flags is updated, the code always returns
an error for case WLAN_CIPHER_SUITE_TKIP. This analysis led me to think
that this is an actual bug, so I sent a patch to fix it:

https://lore.kernel.org/patchwork/patch/1002440/

Notice that this bug has been there since 2015:
commit 26f1fad29ad973b0fb26a9ca3dcb2a73dde781aa


Now, let's take a look at the following warning in net/wireless/chan.c:

net/wireless/chan.c: In function ‘cfg80211_chandef_usable’:
net/wireless/chan.c:748:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
   if (!ht_cap->ht_supported)
      ^
net/wireless/chan.c:750:2: note: here
  case NL80211_CHAN_WIDTH_20_NOHT:
  ^~~~

This is the piece of code at net/wireless/chan.c:740:

 740         case NL80211_CHAN_WIDTH_5:
 741                 width = 5;
 742                 break;
 743         case NL80211_CHAN_WIDTH_10:
 744                 prohibited_flags |= IEEE80211_CHAN_NO_10MHZ;
 745                 width = 10;
 746                 break;
 747         case NL80211_CHAN_WIDTH_20:
 748                 if (!ht_cap->ht_supported)
 749                         return false;
 750         case NL80211_CHAN_WIDTH_20_NOHT:
 751                 prohibited_flags |= IEEE80211_CHAN_NO_20MHZ;
 752                 width = 20;
 753                 break;
 754         case NL80211_CHAN_WIDTH_40:
 755                 width = 40;
 756                 if (!ht_cap->ht_supported)
 757                         return false;
 758                 if (!(ht_cap->cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) ||
 759                     ht_cap->cap & IEEE80211_HT_CAP_40MHZ_INTOLERANT)
 760                         return false;
 761                 if (chandef->center_freq1 < control_freq &&
 762                     chandef->chan->flags & IEEE80211_CHAN_NO_HT40MINUS)
 763                         return false;
 764                 if (chandef->center_freq1 > control_freq &&
 765                     chandef->chan->flags & IEEE80211_CHAN_NO_HT40PLUS)
 766                         return false;
 767                 break;

Notice that the warning was reported at line 748, but I'm including more code
here to make it explicitly clear that I not only focus my atention in a very
narrowed piece of code around the warning.

Now, I don't see anything supicious. The labels NL80211_CHAN_WIDTH_20 and
NL80211_CHAN_WIDTH_20_NOHT seem to be related, and it's even less supicious
when there is an explicit line of code that breaks the switch under certain
conditions, just at the bottom of the "case", as is the case with lines 748
and 749:

 748                 if (!ht_cap->ht_supported)
 749                         return false;

Also, no default case returning an error every time if the code falls through
to the next case. Lastly, I also check the age of the code, but this only after
I have analyzed it as explained above.


I do this analysis for every warning.  Now, when I say I haven't tested the code
is because I don't have any log as evidence for anything. Not that I haven't put
any effort on trying to understand it and its context.  When I started working on
this task there were more than 2000 of these issues, now there are around 600 left.

I have fixed many bugs on the way, so a good amount of work is being invested on
this, and it's paying off. :)


Now, let me ask you this question:

It would be easier for you to review this patch if I turn it into a series?

I can do that without a problem.

Thanks!
--
Gustavo

Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Gustavo A. R. Silva]

On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote:
> 
> On 10/23/18 9:01 AM, Johannes Berg wrote:
>> On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
>>> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>>> where we are expecting to fall through.
>>>
>>> Warning level 3 was used: -Wimplicit-fallthrough=3
>>>
>>> This code was not tested and GCC 7.2.0 was used to compile it.
>>
>> Look, I'm not going to make this any clearer: I'm not applying patches
>> like that where you've invested no effort whatsoever on verifying that
>> they're correct.
>>
> 
> How do you suggest me to verify that every part is correct in this type
> of patches?
> 

BTW... I'm under the impression you think that I don't even look at
the code. Is that correct?

I've been working on this for quite a while, and in every case I try to
understand the code in terms of the context in which every warning is
reported.

I look for dependencies between variables in adjacent switch cases, that could
make think it might be OK for some of those cases to fall through to the one
below. I check the names of the case labels to see if there might be any relation
between them, or if they are totally diferent as it might be the case for labels
that indicate the transmision(FOO_TX) or reception of data(FOO_RX). Something
similar to the latter is the case with on/off logic (FOO_ON/FOO_OFF). These are
some of the things I review in the code, so I can have an idea if the warning is
a false positive or an actual bug.

Here is a bug I found yesterday at drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c

5690         case WLAN_CIPHER_SUITE_CCMP:
5691                 key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
5692                 break;
5693         case WLAN_CIPHER_SUITE_TKIP:
5694                 key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
5695         default:
5696                 return -EOPNOTSUPP;
5697         }

Notice how the absence of a break statement is very suspicious in case
WLAN_CIPHER_SUITE_TKIP, because the code for case WLAN_CIPHER_SUITE_CCMP
is pretty similar and in that case there is a break at the bottom. Now,
that's not the only thing that looks supicious: in the absence of a break,
the code falls through to the default case, which returns the error value
-EOPNOTSUPP. So, even when key->flags is updated, the code always returns
an error for case WLAN_CIPHER_SUITE_TKIP. This analysis led me to think
that this is an actual bug, so I sent a patch to fix it:

https://lore.kernel.org/patchwork/patch/1002440/

Notice that this bug has been there since 2015:
commit 26f1fad29ad973b0fb26a9ca3dcb2a73dde781aa


Now, let's take a look at the following warning in net/wireless/chan.c:

net/wireless/chan.c: In function ‘cfg80211_chandef_usable’:
net/wireless/chan.c:748:6: warning: this statement may fall through [-Wimplicit-fallthrough=]
   if (!ht_cap->ht_supported)
      ^
net/wireless/chan.c:750:2: note: here
  case NL80211_CHAN_WIDTH_20_NOHT:
  ^~~~

This is the piece of code at net/wireless/chan.c:740:

 740         case NL80211_CHAN_WIDTH_5:
 741                 width = 5;
 742                 break;
 743         case NL80211_CHAN_WIDTH_10:
 744                 prohibited_flags |= IEEE80211_CHAN_NO_10MHZ;
 745                 width = 10;
 746                 break;
 747         case NL80211_CHAN_WIDTH_20:
 748                 if (!ht_cap->ht_supported)
 749                         return false;
 750         case NL80211_CHAN_WIDTH_20_NOHT:
 751                 prohibited_flags |= IEEE80211_CHAN_NO_20MHZ;
 752                 width = 20;
 753                 break;
 754         case NL80211_CHAN_WIDTH_40:
 755                 width = 40;
 756                 if (!ht_cap->ht_supported)
 757                         return false;
 758                 if (!(ht_cap->cap & IEEE80211_HT_CAP_SUP_WIDTH_20_40) ||
 759                     ht_cap->cap & IEEE80211_HT_CAP_40MHZ_INTOLERANT)
 760                         return false;
 761                 if (chandef->center_freq1 < control_freq &&
 762                     chandef->chan->flags & IEEE80211_CHAN_NO_HT40MINUS)
 763                         return false;
 764                 if (chandef->center_freq1 > control_freq &&
 765                     chandef->chan->flags & IEEE80211_CHAN_NO_HT40PLUS)
 766                         return false;
 767                 break;

Notice that the warning was reported at line 748, but I'm including more code
here to make it explicitly clear that I not only focus my atention in a very
narrowed piece of code around the warning.

Now, I don't see anything supicious. The labels NL80211_CHAN_WIDTH_20 and
NL80211_CHAN_WIDTH_20_NOHT seem to be related, and it's even less supicious
when there is an explicit line of code that breaks the switch under certain
conditions, just at the bottom of the "case", as is the case with lines 748
and 749:

 748                 if (!ht_cap->ht_supported)
 749                         return false;

Also, no default case returning an error every time if the code falls through
to the next case. Lastly, I also check the age of the code, but this only after
I have analyzed it as explained above.


I do this analysis for every warning.  Now, when I say I haven't tested the code
is because I don't have any log as evidence for anything. Not that I haven't put
any effort on trying to understand it and its context.  When I started working on
this task there were more than 2000 of these issues, now there are around 600 left.

I have fixed many bugs on the way, so a good amount of work is being invested on
this, and it's paying off. :)


Now, let me ask you this question:

It would be easier for you to review this patch if I turn it into a series?

I can do that without a problem.

Thanks!
--
Gustavo

Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Johannes Berg]

On Tue, 2018-10-23 at 12:58 +0200, Gustavo A. R. Silva wrote:
> On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote:
> > 
> > On 10/23/18 9:01 AM, Johannes Berg wrote:
> > > On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
> > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > > where we are expecting to fall through.
> > > > 
> > > > Warning level 3 was used: -Wimplicit-fallthrough=3
> > > > 
> > > > This code was not tested and GCC 7.2.0 was used to compile it.
> > > 
> > > Look, I'm not going to make this any clearer: I'm not applying patches
> > > like that where you've invested no effort whatsoever on verifying that
> > > they're correct.
> > > 
> > 
> > How do you suggest me to verify that every part is correct in this type
> > of patches?
> > 
> 
> BTW... I'm under the impression you think that I don't even look at
> the code. Is that correct?

That's what your commit log looks like, yes. This is your full commit
log:

   In preparation to enabling -Wimplicit-fallthrough, mark switch cases
   where we are expecting to fall through.

   Warning level 3 was used: -Wimplicit-fallthrough=3

   This code was not tested and GCC 7.2.0 was used to compile it.

   For all I know, you could've run spatch to add the comments wherever
   there was no break, and then compiled it once.

   > I've been working on this for quite a while, and in every case I try to
> understand the code in terms of the context in which every warning is
> reported.

That's great.

> Here is a bug I found yesterday at drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
> 
> 5690         case WLAN_CIPHER_SUITE_CCMP:
> 5691                 key->flags |= IEEE80211_KEY_FLAG_SW_MGMT_TX;
> 5692                 break;
> 5693         case WLAN_CIPHER_SUITE_TKIP:
> 5694                 key->flags |= IEEE80211_KEY_FLAG_GENERATE_MMIC;
> 5695         default:
> 5696                 return -EOPNOTSUPP;
> 5697         }

Indeed, that looks like a bug, although kinda benign since it just means
TKIP will always use software crypto and TKIP is slow anyway ;-)

> I do this analysis for every warning.  Now, when I say I haven't tested the code
> is because I don't have any log as evidence for anything. Not that I haven't put
> any effort on trying to understand it and its context.  When I started working on
> this task there were more than 2000 of these issues, now there are around 600 left.
> 
> I have fixed many bugs on the way, so a good amount of work is being invested on
> this, and it's paying off. :)

:-)

> Now, let me ask you this question:
> 
> It would be easier for you to review this patch if I turn it into a series?
> 
> I can do that without a problem.

I'd be happy if you were to actually just mention that you've looked at
them, and found them to be expected fall throughs. I'll still review
them, but without that information I feel like I'm doing the first round
of reviews this code ever got.

johannes

Re: [PATCH v2] wireless: mark expected switch fall-throughs

[Julian Calaby]

Hi Gustavo,

On Wed, Oct 24, 2018 at 7:36 AM Johannes Berg <johannes@sipsolutions.net> wrote:
>
> On Tue, 2018-10-23 at 12:58 +0200, Gustavo A. R. Silva wrote:
> > On 10/23/18 10:59 AM, Gustavo A. R. Silva wrote:
> > >
> > > On 10/23/18 9:01 AM, Johannes Berg wrote:
> > > > On Tue, 2018-10-23 at 02:13 +0200, Gustavo A. R. Silva wrote:
> > > > > In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> > > > > where we are expecting to fall through.
> > > > >
> > > > > Warning level 3 was used: -Wimplicit-fallthrough=3
> > > > >
> > > > > This code was not tested and GCC 7.2.0 was used to compile it.
> > > >
> > > > Look, I'm not going to make this any clearer: I'm not applying patches
> > > > like that where you've invested no effort whatsoever on verifying that
> > > > they're correct.
> > > >
> > >
> > > How do you suggest me to verify that every part is correct in this type
> > > of patches?
> > >
> >
> > BTW... I'm under the impression you think that I don't even look at
> > the code. Is that correct?
>
> That's what your commit log looks like, yes. This is your full commit
> log:
>
>    In preparation to enabling -Wimplicit-fallthrough, mark switch cases
>    where we are expecting to fall through.
>
>    Warning level 3 was used: -Wimplicit-fallthrough=3
>
>    This code was not tested and GCC 7.2.0 was used to compile it.
>
>    For all I know, you could've run spatch to add the comments wherever
>    there was no break, and then compiled it once.

It might also help to split these up by the "type" of change. For
example this patch contains:

1. A whole lot of places where there's a "if (condition) { break; }"
just before the fall through

You could add a line describing the logic there to the commit message,
maybe something like:

As there's a conditional break at the end of the case, the fall
through appears to be intentional.


2. Reformatting an existing comment

These should definitely be split out as they're just reformatting
comments to match gcc's expectations. You could describe this like:

Reformat existing fall through messages into the format expected by gcc.


Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/