* Checkpatch warnings on switch case fallthru
@ 2016-04-07 22:23 Harvey, Ryan
2016-04-09 9:06 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Harvey, Ryan @ 2016-04-07 22:23 UTC (permalink / raw)
To: alsa-devel@alsa-project.org
Hello,
I receive a set of checkpatch warnings against my switch fallthru
structure in readable_registers(). I¹m able to eliminate these warnings
with a preceding comment, but that does messy the code. Is there a method
better than switch for accomplishing this?
static bool cs43130_readable_register(struct device *dev, unsigned int reg)
{
switch (reg) {
case CS43130_DEVID_AB ... CS43130_SYS_CLK_CTL_1:
case CS43130_PWDN_CTL:
case CS43130_PLL_SET_1 ... CS43130_PLL_SET_6:
case CS43130_PLL_SET_7:
case CS43130_PLL_SET_8:
case CS43130_PLL_SET_9:
case CS43130_PLL_SET_10:
case CS43130_CLKOUT_CTL:
case CS43130_ASP_NUM_1 ... CS43130_ASP_FRAME_CONF:
case CS43130_HP_OUT_CTL_1:
case CS43130_PCM_FILT_OPT ... CS43130_PCM_PATH_CTL_2:
case CS43130_CLASS_H_CTL:
case CS43130_HP_DETECT ... CS43130_HP_STATUS:
case CS43130_HP_LOAD_1:
case CS43130_HP_LOAD_STAT:
case CS43130_INT_MASK_1 ... CS43130_INT_MASK_5:
return true;
default:
return false;
}
}
140: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_PLL_SET_7:
141: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_PLL_SET_8:
142: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_PLL_SET_9:
143: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_PLL_SET_10:
144: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_CLKOUT_CTL:
152: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_HP_OUT_CTL_1:
154: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_CLASS_H_CTL:
156: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_HP_LOAD_1:
160: [WARNING] Possible switch case/default not preceeded by break or
fallthrough comment
+ case CS43130_HP_LOAD_STAT:
Thanks
-Ryan
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Checkpatch warnings on switch case fallthru
2016-04-07 22:23 Checkpatch warnings on switch case fallthru Harvey, Ryan
@ 2016-04-09 9:06 ` Takashi Iwai
2016-04-09 16:07 ` Lars-Peter Clausen
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2016-04-09 9:06 UTC (permalink / raw)
To: Harvey, Ryan; +Cc: alsa-devel@alsa-project.org
On Fri, 08 Apr 2016 00:23:03 +0200,
Harvey, Ryan wrote:
>
> Hello,
>
> I receive a set of checkpatch warnings against my switch fallthru
> structure in readable_registers(). I¹m able to eliminate these warnings
> with a preceding comment, but that does messy the code. Is there a method
> better than switch for accomplishing this?
>
> static bool cs43130_readable_register(struct device *dev, unsigned int reg)
> {
> switch (reg) {
> case CS43130_DEVID_AB ... CS43130_SYS_CLK_CTL_1:
> case CS43130_PWDN_CTL:
> case CS43130_PLL_SET_1 ... CS43130_PLL_SET_6:
> case CS43130_PLL_SET_7:
> case CS43130_PLL_SET_8:
> case CS43130_PLL_SET_9:
> case CS43130_PLL_SET_10:
> case CS43130_CLKOUT_CTL:
> case CS43130_ASP_NUM_1 ... CS43130_ASP_FRAME_CONF:
> case CS43130_HP_OUT_CTL_1:
> case CS43130_PCM_FILT_OPT ... CS43130_PCM_PATH_CTL_2:
> case CS43130_CLASS_H_CTL:
> case CS43130_HP_DETECT ... CS43130_HP_STATUS:
> case CS43130_HP_LOAD_1:
> case CS43130_HP_LOAD_STAT:
> case CS43130_INT_MASK_1 ... CS43130_INT_MASK_5:
> return true;
> default:
> return false;
> }
> }
>
>
> 140: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_PLL_SET_7:
> 141: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_PLL_SET_8:
> 142: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_PLL_SET_9:
> 143: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_PLL_SET_10:
> 144: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_CLKOUT_CTL:
> 152: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_HP_OUT_CTL_1:
> 154: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_CLASS_H_CTL:
> 156: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_HP_LOAD_1:
> 160: [WARNING] Possible switch case/default not preceeded by break or
> fallthrough comment
> + case CS43130_HP_LOAD_STAT:
You may ignore the warnings. The purpose of checkpatch is to improve
the code readability, and what it suggests is exactly opposite.
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Checkpatch warnings on switch case fallthru
2016-04-09 9:06 ` Takashi Iwai
@ 2016-04-09 16:07 ` Lars-Peter Clausen
2016-04-09 19:45 ` Joe Perches
2016-04-11 2:26 ` [PATCH] checkpatch: Improve missing break for switch/case tests Joe Perches
0 siblings, 2 replies; 6+ messages in thread
From: Lars-Peter Clausen @ 2016-04-09 16:07 UTC (permalink / raw)
To: Takashi Iwai, Harvey, Ryan; +Cc: Joe Perches, alsa-devel@alsa-project.org
On 04/09/2016 11:06 AM, Takashi Iwai wrote:
> On Fri, 08 Apr 2016 00:23:03 +0200,
> Harvey, Ryan wrote:
>>
>> Hello,
>>
>> I receive a set of checkpatch warnings against my switch fallthru
>> structure in readable_registers(). I¹m able to eliminate these warnings
>> with a preceding comment, but that does messy the code. Is there a method
>> better than switch for accomplishing this?
>>
>> static bool cs43130_readable_register(struct device *dev, unsigned int reg)
>> {
>> switch (reg) {
>> case CS43130_DEVID_AB ... CS43130_SYS_CLK_CTL_1:
>> case CS43130_PWDN_CTL:
>> case CS43130_PLL_SET_1 ... CS43130_PLL_SET_6:
>> case CS43130_PLL_SET_7:
>> case CS43130_PLL_SET_8:
>> case CS43130_PLL_SET_9:
>> case CS43130_PLL_SET_10:
>> case CS43130_CLKOUT_CTL:
>> case CS43130_ASP_NUM_1 ... CS43130_ASP_FRAME_CONF:
>> case CS43130_HP_OUT_CTL_1:
>> case CS43130_PCM_FILT_OPT ... CS43130_PCM_PATH_CTL_2:
>> case CS43130_CLASS_H_CTL:
>> case CS43130_HP_DETECT ... CS43130_HP_STATUS:
>> case CS43130_HP_LOAD_1:
>> case CS43130_HP_LOAD_STAT:
>> case CS43130_INT_MASK_1 ... CS43130_INT_MASK_5:
>> return true;
>> default:
>> return false;
>> }
>> }
>>
>>
>> 140: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_PLL_SET_7:
>> 141: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_PLL_SET_8:
>> 142: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_PLL_SET_9:
>> 143: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_PLL_SET_10:
>> 144: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_CLKOUT_CTL:
>> 152: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_HP_OUT_CTL_1:
>> 154: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_CLASS_H_CTL:
>> 156: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_HP_LOAD_1:
>> 160: [WARNING] Possible switch case/default not preceeded by break or
>> fallthrough comment
>> + case CS43130_HP_LOAD_STAT:
>
> You may ignore the warnings. The purpose of checkpatch is to improve
> the code readability, and what it suggests is exactly opposite.
It's a bug in checkpatch. It seems to get confused by the "...". The
best approach to solving those warnings is to create a fix for checkpatch.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Checkpatch warnings on switch case fallthru
2016-04-09 16:07 ` Lars-Peter Clausen
@ 2016-04-09 19:45 ` Joe Perches
2016-04-11 2:26 ` [PATCH] checkpatch: Improve missing break for switch/case tests Joe Perches
1 sibling, 0 replies; 6+ messages in thread
From: Joe Perches @ 2016-04-09 19:45 UTC (permalink / raw)
To: Lars-Peter Clausen, Takashi Iwai, Harvey, Ryan
Cc: alsa-devel@alsa-project.org
On Sat, 2016-04-09 at 18:07 +0200, Lars-Peter Clausen wrote:
> On 04/09/2016 11:06 AM, Takashi Iwai wrote:
> >
> > On Fri, 08 Apr 2016 00:23:03 +0200,
> > Harvey, Ryan wrote:
> > >
> > >
> > > Hello,
> > >
> > > I receive a set of checkpatch warnings against my switch fallthru
> > > structure in readable_registers(). I¹m able to eliminate these warnings
> > > with a preceding comment, but that does messy the code. Is there a method
> > > better than switch for accomplishing this?
> > >
> > > static bool cs43130_readable_register(struct device *dev, unsigned int reg)
> > > {
> > > switch (reg) {
> > > case CS43130_DEVID_AB ... CS43130_SYS_CLK_CTL_1:
> > > case CS43130_PWDN_CTL:
> > > case CS43130_PLL_SET_1 ... CS43130_PLL_SET_6:
> > > case CS43130_PLL_SET_7:
> > > case CS43130_PLL_SET_8:
> > > case CS43130_PLL_SET_9:
> > > case CS43130_PLL_SET_10:
> > > case CS43130_CLKOUT_CTL:
> > > case CS43130_ASP_NUM_1 ... CS43130_ASP_FRAME_CONF:
> > > case CS43130_HP_OUT_CTL_1:
> > > case CS43130_PCM_FILT_OPT ... CS43130_PCM_PATH_CTL_2:
> > > case CS43130_CLASS_H_CTL:
> > > case CS43130_HP_DETECT ... CS43130_HP_STATUS:
> > > case CS43130_HP_LOAD_1:
> > > case CS43130_HP_LOAD_STAT:
> > > case CS43130_INT_MASK_1 ... CS43130_INT_MASK_5:
> > > return true;
> > > default:
> > > return false;
> > > }
> > > }
> > >
> > >
> > > 140: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_PLL_SET_7:
> > > 141: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_PLL_SET_8:
> > > 142: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_PLL_SET_9:
> > > 143: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_PLL_SET_10:
> > > 144: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_CLKOUT_CTL:
> > > 152: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_HP_OUT_CTL_1:
> > > 154: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_CLASS_H_CTL:
> > > 156: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_HP_LOAD_1:
> > > 160: [WARNING] Possible switch case/default not preceeded by break or
> > > fallthrough comment
> > > + case CS43130_HP_LOAD_STAT:
> > You may ignore the warnings. The purpose of checkpatch is to improve
> > the code readability, and what it suggests is exactly opposite.
> It's a bug in checkpatch. It seems to get confused by the "...". The
> best approach to solving those warnings is to create a fix for checkpatch.
Can you send me the checkpatch.pl file you are using please?
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] checkpatch: Improve missing break for switch/case tests
2016-04-09 16:07 ` Lars-Peter Clausen
2016-04-09 19:45 ` Joe Perches
@ 2016-04-11 2:26 ` Joe Perches
2016-04-11 5:04 ` Joe Perches
1 sibling, 1 reply; 6+ messages in thread
From: Joe Perches @ 2016-04-11 2:26 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft
Cc: Takashi Iwai, alsa-devel@alsa-project.org, Lars-Peter Clausen,
LKML, Harvey, Ryan
The current switch/case test doesn't handle ... case labels like:
switch (foo) {
case bar ... baz:
etc...
}
Add a specific regex for that form and the default label.
Use the regex where a case label is tested.
Improve the missing break/fall-through test by only reporting
the first case label missing the break not every case label
where there isn't a preceding break/fall-through.
Show the line above the case label when reporting
the possible defect.
Signed-off-by: Joe Perches <joe@perches.com>
Reported-by: Lars-Peter Clausen <lars@metafoo.de>
---
scripts/checkpatch.pl | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e3d9c34..216e4a1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -331,6 +331,11 @@ our $Operators = qr{
}x;
our $c90_Keywords = qr{do|for|while|if|else|return|goto|continue|switch|default|case|break}x;
+our $case_label = qr{
+ (?:case\s+(?:$Ident|$Constant)
+ (?:\s*\.\.\.\s*\s*(?:$Ident|$Constant))? |
+ default)\s*:
+ }x;
our $BasicType;
our $NonptrType;
@@ -4417,7 +4422,7 @@ sub process {
$herecurr);
}
# case and default should not have general statements after them
- if ($line =~ /^.\s*(?:case\s*.*|default\s*):/g &&
+ if ($line =~ /^.\s*$case_label/g &&
$line !~ /\G(?:
(?:\s*$;*)(?:\s*{)?(?:\s*$;*)(?:\s*\\)?\s*$|
\s*return\s+
@@ -5648,27 +5653,28 @@ sub process {
}
# check for case / default statements not preceded by break/fallthrough/switch
- if ($line =~ /^.\s*(?:case\s+(?:$Ident|$Constant)\s*|default):/) {
+ if ($line =~ /^.\s*$case_label/ &&
+ $prevline !~ /^.\s*$case_label/) {
my $has_break = 0;
my $has_statement = 0;
my $count = 0;
my $prevline = $linenr;
- while ($prevline > 1 && ($file || $count < 3) && !$has_break) {
+ while ($prevline > 1 && ($file || $count < 3) && !$has_break && !$has_statement) {
$prevline--;
my $rline = $rawlines[$prevline - 1];
my $fline = $lines[$prevline - 1];
last if ($fline =~ /^\@\@/);
next if ($fline =~ /^\-/);
- next if ($fline =~ /^.(?:\s*(?:case\s+(?:$Ident|$Constant)[\s$;]*|default):[\s$;]*)*$/);
+ next if ($fline =~ /^.(?:\s*${case_label}[\s$;]*)*$/);
$has_break = 1 if ($rline =~ /fall[\s_-]*(through|thru)/i);
next if ($fline =~ /^.[\s$;]*$/);
- $has_statement = 1;
$count++;
$has_break = 1 if ($fline =~ /\bswitch\b|\b(?:break\s*;[\s$;]*$|return\b|goto\b|continue\b)/);
+ $has_statement = 1;
}
if (!$has_break && $has_statement) {
WARN("MISSING_BREAK",
- "Possible switch case/default not preceeded by break or fallthrough comment\n" . $herecurr);
+ "Possible switch case/default not preceeded by break or fallthrough comment\n" . $hereprev);
}
}
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] checkpatch: Improve missing break for switch/case tests
2016-04-11 2:26 ` [PATCH] checkpatch: Improve missing break for switch/case tests Joe Perches
@ 2016-04-11 5:04 ` Joe Perches
0 siblings, 0 replies; 6+ messages in thread
From: Joe Perches @ 2016-04-11 5:04 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft
Cc: Takashi Iwai, alsa-devel@alsa-project.org, Lars-Peter Clausen,
LKML, Harvey, Ryan
On Sun, 2016-04-10 at 19:26 -0700, Joe Perches wrote:
> The current switch/case test doesn't handle ... case labels like:
>
> switch (foo) {
> case bar ... baz:
> etc...
> }
>
> Add a specific regex for that form and the default label.
> Use the regex where a case label is tested.
Please ignore, the patch is defective as
it has too many false positives.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-04-11 5:04 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-07 22:23 Checkpatch warnings on switch case fallthru Harvey, Ryan
2016-04-09 9:06 ` Takashi Iwai
2016-04-09 16:07 ` Lars-Peter Clausen
2016-04-09 19:45 ` Joe Perches
2016-04-11 2:26 ` [PATCH] checkpatch: Improve missing break for switch/case tests Joe Perches
2016-04-11 5:04 ` Joe Perches
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).