* [PATCH][next] greybus: Use fallthrough pseudo-keyword @ 2020-07-27 18:32 Gustavo A. R. Silva 2020-07-28 22:37 ` Alex Elder 0 siblings, 1 reply; 7+ messages in thread From: Gustavo A. R. Silva @ 2020-07-27 18:32 UTC (permalink / raw) To: Johan Hovold, Alex Elder, Greg Kroah-Hartman Cc: greybus-dev, linux-kernel, Gustavo A. R. Silva Replace the existing /* fall through */ comments and its variants with the new pseudo-keyword macro fallthrough[1]. [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- drivers/greybus/es2.c | 2 +- drivers/greybus/interface.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c index 366716f11b1a..1df6ab5d339d 100644 --- a/drivers/greybus/es2.c +++ b/drivers/greybus/es2.c @@ -759,7 +759,7 @@ static int check_urb_status(struct urb *urb) case -EOVERFLOW: dev_err(dev, "%s: overflow actual length is %d\n", __func__, urb->actual_length); - /* fall through */ + fallthrough; case -ECONNRESET: case -ENOENT: case -ESHUTDOWN: diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c index 67dbe6fda9a1..58ea374d8aaa 100644 --- a/drivers/greybus/interface.c +++ b/drivers/greybus/interface.c @@ -1233,7 +1233,7 @@ int gb_interface_add(struct gb_interface *intf) case GB_INTERFACE_TYPE_GREYBUS: dev_info(&intf->dev, "GMP VID=0x%08x, PID=0x%08x\n", intf->vendor_id, intf->product_id); - /* fall-through */ + fallthrough; case GB_INTERFACE_TYPE_UNIPRO: dev_info(&intf->dev, "DDBL1 Manufacturer=0x%08x, Product=0x%08x\n", intf->ddbl1_manufacturer_id, -- 2.27.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH][next] greybus: Use fallthrough pseudo-keyword 2020-07-27 18:32 [PATCH][next] greybus: Use fallthrough pseudo-keyword Gustavo A. R. Silva @ 2020-07-28 22:37 ` Alex Elder 2020-07-29 0:25 ` Joe Perches ` (2 more replies) 0 siblings, 3 replies; 7+ messages in thread From: Alex Elder @ 2020-07-28 22:37 UTC (permalink / raw) To: Gustavo A. R. Silva, Johan Hovold, Alex Elder, Greg Kroah-Hartman Cc: greybus-dev, linux-kernel On 7/27/20 1:32 PM, Gustavo A. R. Silva wrote: > Replace the existing /* fall through */ comments and its variants with > the new pseudo-keyword macro fallthrough[1]. > > [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through Thanks for the patch. It looks good, but it raises another question I'd like discussion on. It seems that Johan likes default (or final) cases in switch statements without a "break" statement. Viresh and Bryan appear to be fond of this too. It's pedantic, but I don't like that. Am I wrong? --> Johan/Viresh/Bryan would you please comment? If they aren't convincing (or don't care) I think break statements should also be added here: - In gb_interface_read_and_clear_init_status() for the default case - In gb_interface_activate() for the default case. - In gb_svc_process_deferred_request() for the default case But let's wait to see what Johan (et al) says. If you don't want to do that, say so and I'll do it later. I looked at the code in drivers/staging/greybus/ and saw no need to add a "fallthrough" anywhere, but: - In fw_mgmt_backend_fw_version_operation() Viresh seems to have skipped the break in the fault statement - In gb_uart_request_handler() Bryan did this too. Depending on discussion, these could be fixed in a separate patch as well. These cases aren't found by "checkpatch.pl" because it only looks at case "blocks" that are followed by another case. So the last case isn't checked. -Alex > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > drivers/greybus/es2.c | 2 +- > drivers/greybus/interface.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c > index 366716f11b1a..1df6ab5d339d 100644 > --- a/drivers/greybus/es2.c > +++ b/drivers/greybus/es2.c > @@ -759,7 +759,7 @@ static int check_urb_status(struct urb *urb) > case -EOVERFLOW: > dev_err(dev, "%s: overflow actual length is %d\n", > __func__, urb->actual_length); > - /* fall through */ > + fallthrough; > case -ECONNRESET: > case -ENOENT: > case -ESHUTDOWN: > diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c > index 67dbe6fda9a1..58ea374d8aaa 100644 > --- a/drivers/greybus/interface.c > +++ b/drivers/greybus/interface.c > @@ -1233,7 +1233,7 @@ int gb_interface_add(struct gb_interface *intf) > case GB_INTERFACE_TYPE_GREYBUS: > dev_info(&intf->dev, "GMP VID=0x%08x, PID=0x%08x\n", > intf->vendor_id, intf->product_id); > - /* fall-through */ > + fallthrough; > case GB_INTERFACE_TYPE_UNIPRO: > dev_info(&intf->dev, "DDBL1 Manufacturer=0x%08x, Product=0x%08x\n", > intf->ddbl1_manufacturer_id, > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] greybus: Use fallthrough pseudo-keyword 2020-07-28 22:37 ` Alex Elder @ 2020-07-29 0:25 ` Joe Perches 2020-07-29 10:51 ` [greybus-dev] " Viresh Kumar 2020-08-05 13:14 ` Alex Elder 2 siblings, 0 replies; 7+ messages in thread From: Joe Perches @ 2020-07-29 0:25 UTC (permalink / raw) To: Alex Elder, Gustavo A. R. Silva, Johan Hovold, Alex Elder, Greg Kroah-Hartman Cc: greybus-dev, linux-kernel On Tue, 2020-07-28 at 17:37 -0500, Alex Elder wrote: > On 7/27/20 1:32 PM, Gustavo A. R. Silva wrote: > > Replace the existing /* fall through */ comments and its variants with > > the new pseudo-keyword macro fallthrough[1]. > > > > [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through > > Thanks for the patch. It looks good, but it raises > another question I'd like discussion on. > > It seems that Johan likes default (or final) cases in > switch statements without a "break" statement. Viresh > and Bryan appear to be fond of this too. > > It's pedantic, but I don't like that. Am I wrong? No, you are not wrong. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [greybus-dev] [PATCH][next] greybus: Use fallthrough pseudo-keyword 2020-07-28 22:37 ` Alex Elder 2020-07-29 0:25 ` Joe Perches @ 2020-07-29 10:51 ` Viresh Kumar 2020-07-29 12:15 ` Alex Elder 2020-08-05 13:14 ` Alex Elder 2 siblings, 1 reply; 7+ messages in thread From: Viresh Kumar @ 2020-07-29 10:51 UTC (permalink / raw) To: Alex Elder Cc: Gustavo A. R. Silva, Johan Hovold, Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-kernel On 28-07-20, 17:37, Alex Elder wrote: > On 7/27/20 1:32 PM, Gustavo A. R. Silva wrote: > > Replace the existing /* fall through */ comments and its variants with > > the new pseudo-keyword macro fallthrough[1]. > > > > [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through > > Thanks for the patch. It looks good, but it raises > another question I'd like discussion on. > > It seems that Johan likes default (or final) cases in > switch statements without a "break" statement. Viresh > and Bryan appear to be fond of this too. > > It's pedantic, but I don't like that. Am I wrong? > --> Johan/Viresh/Bryan would you please comment? I am not fond of them as they aren't required for the working of the code. It is a bit like using an empty return statement for a routine with void return type, though it surely adds some consistency to the switch case. But if people really feel it must be there, then its fine :) -- viresh ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [greybus-dev] [PATCH][next] greybus: Use fallthrough pseudo-keyword 2020-07-29 10:51 ` [greybus-dev] " Viresh Kumar @ 2020-07-29 12:15 ` Alex Elder 0 siblings, 0 replies; 7+ messages in thread From: Alex Elder @ 2020-07-29 12:15 UTC (permalink / raw) To: Viresh Kumar Cc: Alex Elder, Gustavo A. R. Silva, linux-kernel, greybus-dev, Johan Hovold On 7/29/20 5:51 AM, Viresh Kumar wrote: > On 28-07-20, 17:37, Alex Elder wrote: >> On 7/27/20 1:32 PM, Gustavo A. R. Silva wrote: >>> Replace the existing /* fall through */ comments and its variants with >>> the new pseudo-keyword macro fallthrough[1]. >>> >>> [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through >> >> Thanks for the patch. It looks good, but it raises >> another question I'd like discussion on. >> >> It seems that Johan likes default (or final) cases in >> switch statements without a "break" statement. Viresh >> and Bryan appear to be fond of this too. >> >> It's pedantic, but I don't like that. Am I wrong? >> --> Johan/Viresh/Bryan would you please comment? > > I am not fond of them as they aren't required for the working of the code. It is > a bit like using an empty return statement for a routine with void return type, > though it surely adds some consistency to the switch case. I understand this perspective, and it's exactly why I wanted to have a conversation about it (rather than just saying it should be fixed). As similar example, I don't like unnecessary parentheses, but sometimes it's a good idea to have them. Thanks. -Alex > But if people really feel it must be there, then its fine :) > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] greybus: Use fallthrough pseudo-keyword 2020-07-28 22:37 ` Alex Elder 2020-07-29 0:25 ` Joe Perches 2020-07-29 10:51 ` [greybus-dev] " Viresh Kumar @ 2020-08-05 13:14 ` Alex Elder 2020-08-05 16:21 ` Johan Hovold 2 siblings, 1 reply; 7+ messages in thread From: Alex Elder @ 2020-08-05 13:14 UTC (permalink / raw) To: Gustavo A. R. Silva, Johan Hovold, Alex Elder, Greg Kroah-Hartman Cc: greybus-dev, linux-kernel On 7/28/20 5:37 PM, Alex Elder wrote: > On 7/27/20 1:32 PM, Gustavo A. R. Silva wrote: >> Replace the existing /* fall through */ comments and its variants with >> the new pseudo-keyword macro fallthrough[1]. >> >> [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through > > Thanks for the patch. It looks good, but it raises > another question I'd like discussion on. It's been a week, and we heard back from Viresh (and Joe) on this, but no one else. Viresh left out the break statement on the last case of the switch statement intentionally, arguing that it is not needed (much like a return statement at the end of a void function). But he doesn't feel strongly enough insist it should stay that way. I'm sure the others omitted the break statement intentionally as well. Given no strong pushback, I'll ask you (Gustavo) to post a second patch adding the missing break statements I described (and look for any others I might have missed). If you would prefer not to do that, just say so, and I will send out such a patch myself. On your original patch, it looks good to me. Thank you. Reviewed-by: Alex Elder <elder@linaro.org> > It seems that Johan likes default (or final) cases in > switch statements without a "break" statement. Viresh > and Bryan appear to be fond of this too. > > It's pedantic, but I don't like that. Am I wrong? > --> Johan/Viresh/Bryan would you please comment? > > If they aren't convincing (or don't care) I think break > statements should also be added here: > - In gb_interface_read_and_clear_init_status() for the > default case > - In gb_interface_activate() for the default case. > - In gb_svc_process_deferred_request() for the default > case > > But let's wait to see what Johan (et al) says. If you > don't want to do that, say so and I'll do it later. > > I looked at the code in drivers/staging/greybus/ and saw > no need to add a "fallthrough" anywhere, but: > - In fw_mgmt_backend_fw_version_operation() Viresh > seems to have skipped the break in the fault statement > - In gb_uart_request_handler() Bryan did this too. > > Depending on discussion, these could be fixed in a > separate patch as well. > > These cases aren't found by "checkpatch.pl" because it only > looks at case "blocks" that are followed by another case. > So the last case isn't checked. > > -Alex > >> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> >> --- >> drivers/greybus/es2.c | 2 +- >> drivers/greybus/interface.c | 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/greybus/es2.c b/drivers/greybus/es2.c >> index 366716f11b1a..1df6ab5d339d 100644 >> --- a/drivers/greybus/es2.c >> +++ b/drivers/greybus/es2.c >> @@ -759,7 +759,7 @@ static int check_urb_status(struct urb *urb) >> case -EOVERFLOW: >> dev_err(dev, "%s: overflow actual length is %d\n", >> __func__, urb->actual_length); >> - /* fall through */ >> + fallthrough; >> case -ECONNRESET: >> case -ENOENT: >> case -ESHUTDOWN: >> diff --git a/drivers/greybus/interface.c b/drivers/greybus/interface.c >> index 67dbe6fda9a1..58ea374d8aaa 100644 >> --- a/drivers/greybus/interface.c >> +++ b/drivers/greybus/interface.c >> @@ -1233,7 +1233,7 @@ int gb_interface_add(struct gb_interface *intf) >> case GB_INTERFACE_TYPE_GREYBUS: >> dev_info(&intf->dev, "GMP VID=0x%08x, PID=0x%08x\n", >> intf->vendor_id, intf->product_id); >> - /* fall-through */ >> + fallthrough; >> case GB_INTERFACE_TYPE_UNIPRO: >> dev_info(&intf->dev, "DDBL1 Manufacturer=0x%08x, Product=0x%08x\n", >> intf->ddbl1_manufacturer_id, >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH][next] greybus: Use fallthrough pseudo-keyword 2020-08-05 13:14 ` Alex Elder @ 2020-08-05 16:21 ` Johan Hovold 0 siblings, 0 replies; 7+ messages in thread From: Johan Hovold @ 2020-08-05 16:21 UTC (permalink / raw) To: Alex Elder Cc: Gustavo A. R. Silva, Johan Hovold, Alex Elder, Greg Kroah-Hartman, greybus-dev, linux-kernel On Wed, Aug 05, 2020 at 08:14:47AM -0500, Alex Elder wrote: > On 7/28/20 5:37 PM, Alex Elder wrote: > > On 7/27/20 1:32 PM, Gustavo A. R. Silva wrote: > >> Replace the existing /* fall through */ comments and its variants with > >> the new pseudo-keyword macro fallthrough[1]. > >> > >> [1] https://www.kernel.org/doc/html/v5.7/process/deprecated.html?highlight=fallthrough#implicit-switch-case-fall-through > > > > Thanks for the patch. It looks good, but it raises > > another question I'd like discussion on. Sorry about the late reply on this. > It's been a week, and we heard back from Viresh (and Joe) on > this, but no one else. Viresh left out the break statement on > the last case of the switch statement intentionally, arguing > that it is not needed (much like a return statement at the end > of a void function). But he doesn't feel strongly enough > insist it should stay that way. I'm sure the others omitted > the break statement intentionally as well. I really don't mind break statements in the final case and often do add them, but I'm a bit reluctant to suggest that this is something that need "fixing". There are a ton of these all over the kernel, and I think we have too many opportunities for people to do mechanical clean ups already. Especially after Gustavo's work, the only real argument for adding them is mostly moot as the compiler would catch it if anyone adds a new case and forgets about the break statement. > Given no strong pushback, I'll ask you (Gustavo) to post a > second patch adding the missing break statements I described > (and look for any others I might have missed). If you would > prefer not to do that, just say so, and I will send out such > a patch myself. I'd probably just leave it as is, if only to not inspire others to send copy-cat "fixes". Johan ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-08-05 20:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-27 18:32 [PATCH][next] greybus: Use fallthrough pseudo-keyword Gustavo A. R. Silva 2020-07-28 22:37 ` Alex Elder 2020-07-29 0:25 ` Joe Perches 2020-07-29 10:51 ` [greybus-dev] " Viresh Kumar 2020-07-29 12:15 ` Alex Elder 2020-08-05 13:14 ` Alex Elder 2020-08-05 16:21 ` Johan Hovold
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.