Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1619319 > unrolled thread

[bug report] checkpatch: if statement does not need to be enclosed in parentheses

Back to article view | Back to linux.kernel

Contents

  [bug report] checkpatch: if statement does not need to be enclosed  in parentheses Alfonso Lima <alfonsolimaastor@gmail.com> - 2017-04-08 18:10 +0200
    Re: [bug report] checkpatch: if statement does not need to be  enclosed in parentheses Joe Perches <joe@perches.com> - 2017-04-08 19:40 +0200
      Re: [bug report] checkpatch: if statement does not need to be  enclosed in parentheses Andreas Mohr <andi@lisas.de> - 2017-04-09 11:10 +0200
        Re: [PATCH 2/2] checkpatch: Improve  MULTISTATEMENT_MACRO_USE_DO_WHILE test Joe Perches <joe@perches.com> - 2017-04-09 19:50 +0200
        [PATCH 1/2] checkpatch: Clarify the EMBEDDED_FUNCTION_NAME message Joe Perches <joe@perches.com> - 2017-04-09 19:50 +0200

#1619319 — [bug report] checkpatch: if statement does not need to be enclosed in parentheses

Hi,

In current linux-next, if you run the script on this piece of
code:

#define FOO(a)				\
	if (a) {			\
		something();		\
		something_else();	\
	}

You should get:

ERROR: Macros with complex values should be enclosed in parentheses
#1: FILE: foo.c:1:
+#define FOO(a)				\
+	if (a) {			\
+		something();		\
+		something_else();	\
+	}

We could silence checkpatch.pl using "do {} while ()" around the
if statement. However, the "if () {}" statement should be
enough. If someone could confirm this, I'll go and fix it.

Thanks,
Alfonso

[toc] | [next] | [standalone]

#1619348 — Re: [bug report] checkpatch: if statement does not need to be enclosed in parentheses

On Sat, 2017-04-08 at 17:07 +0100, Alfonso Lima wrote:
> Hi,
> 
> In current linux-next, if you run the script on this piece of
> code:
> 
> #define FOO(a)				\
> 	if (a) {			\
> 		something();		\
> 		something_else();	\
> 	}
> 
> You should get:
> 
> ERROR: Macros with complex values should be enclosed in parentheses
> #1: FILE: foo.c:1:
> +#define FOO(a)				\
> +	if (a) {			\
> +		something();		\
> +		something_else();	\
> +	}
> 
> We could silence checkpatch.pl using "do {} while ()" around the
> if statement. However, the "if () {}" statement should be
> enough. If someone could confirm this, I'll go and fix it.

Multiple if/else use is the reason do {} while (0) is suggested.

	if (bar())
		FOO(a);
	else
		baz(b);
 

[toc] | [prev] | [next] | [standalone]

#1619462 — Re: [bug report] checkpatch: if statement does not need to be enclosed in parentheses

On Sat, Apr 08, 2017 at 10:33:19AM -0700, Joe Perches wrote:
> On Sat, 2017-04-08 at 17:07 +0100, Alfonso Lima wrote:
> > Hi,
> > 
> > In current linux-next, if you run the script on this piece of
> > code:
> > 
> > #define FOO(a)				\
> > 	if (a) {			\
> > 		something();		\
> > 		something_else();	\
> > 	}
> > 
> > You should get:
> > 
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #1: FILE: foo.c:1:
> > +#define FOO(a)				\
> > +	if (a) {			\
> > +		something();		\
> > +		something_else();	\
> > +	}
> > 
> > We could silence checkpatch.pl using "do {} while ()" around the
> > if statement. However, the "if () {}" statement should be
> > enough. If someone could confirm this, I'll go and fix it.
> 
> Multiple if/else use is the reason do {} while (0) is suggested.
> 
> 	if (bar())
> 		FOO(a);
> 	else
> 		baz(b);
>  

So, if I read this right checkpatch.pl is generating a correct complaint
in this case after all?

However, then we seem to have the following issues remaining:
- "Macros with complex values should be enclosed in parentheses" != "do {} while (0) is suggested" --> this is an imprecision which may cause confusion
- *further* cases of this issue occurring may just as well suggest again
  "fixing" (i.e., "damaging") checkpatch.pl rather than
  simply obeying (following through on) its correct diagnosis
  --> checkpatch.pl behaviour is confusing enough that
  it may prompt incorrect core handling "repair actions",
  and this danger will remain permanently until
  checkpatch.pl reporting is improved to be
  less confusing/insufficient about its diagnosis

Or perhaps this imprecision here
again simply is due to
the very frequently observable case of
saying "thou shan't do this" without then also
giving some tried-and-true *reason(s)* for such advice.
("ERROR: Macros with complex values should be enclosed in parentheses")

HTH,

Andreas Mohr

[toc] | [prev] | [next] | [standalone]

#1619525 — Re: [PATCH 2/2] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test

On Sun, 2017-04-09 at 10:45 -0700, Joe Perches wrote:
> checkpatch

Sorry, ignore this.

[toc] | [prev] | [next] | [standalone]

#1619526 — [PATCH 1/2] checkpatch: Clarify the EMBEDDED_FUNCTION_NAME message

Try to make the conversion of embedded function names to "%s: ", __func__
a bit clearer.

Add a bit more information to the comment describing the test too.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 089c974aa3a5..e8d8481b24c8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5172,14 +5172,16 @@ sub process {
 			     "break quoted strings at a space character\n" . $hereprev);
 		}
 
-#check for an embedded function name in a string when the function is known
-# as part of a diff.  This does not work for -f --file checking as it
-#depends on patch context providing the function name
+# check for an embedded function name in a string when the function is known
+# This does not work very well for -f --file checking as it depends on patch
+# context providing the function name or a single line form for in-file
+# function declarations
 		if ($line =~ /^\+.*$String/ &&
 		    defined($context_function) &&
-		    get_quoted_string($line, $rawline) =~ /\b$context_function\b/) {
+		    get_quoted_string($line, $rawline) =~ /\b$context_function\b/ &&
+		    length(get_quoted_string($line, $rawline)) != (length($context_function) + 2)) {
 			WARN("EMBEDDED_FUNCTION_NAME",
-			     "Prefer using \"%s\", __func__ to embedded function names\n" . $herecurr);
+			     "Prefer using '\"%s...\", __func__' to using '$context_function', this function's name, in a string\n" . $herecurr);
 		}
 
 # check for spaces before a quoted newline
-- 
2.10.0.rc2.1.g053435c

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web