[BUG] scripts/checkpatch.pl: false positive missing parentheses

[BUG] scripts/checkpatch.pl: false positive missing parentheses

[Heinrich Schuchardt]

This patch creates a false positive:
ERROR: Macros with complex values should be enclosed in parentheses

Here we define a constant that can be used to initialize a structure.
Adding parentheses would lead to a compile time error:
error: braced-group within expression allowed only inside a function

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 foo.h | 8 ++++++++
 1 file changed, 8 insertions(+)
 create mode 100644 foo.h

diff --git a/foo.h b/foo.h
new file mode 100644
index 000000000000..e2cba533f065
--- /dev/null
+++ b/foo.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: BSD-2 */
+#define EFI_ST_DISK_IMG { 0x00003368, { \
+	{0x00000d40, "\x6f\x63\x00\x2f\x2a\x00\x20\x2a"}, /* oc./*. * */ \
+	{0x00000d48, "\x00\x20\x2a\x2f\x0a\x00\x09\x7b"}, /* . */...{ */ \
+	{0x00000d50, "\x30\x78\x25\x30\x38\x7a\x78\x2c"}, /* 0x%08zx, */ \
+	{0x00000d58, "\x20\x22\x00\x5c\x78\x25\x30\x32"}, /*  ".\x%02 */ \
+	{0x00000d60, "\x78\x00\x20\x2a\x2f\x20\x5c\x00"}, /* x. */ \. */ \
+	{0, NULL} } }
-- 
2.17.0

Re: [BUG] scripts/checkpatch.pl: false positive missing parentheses

[Joe Perches]

On Mon, 2018-05-07 at 20:31 +0200, Heinrich Schuchardt wrote:
> This patch creates a false positive:
> ERROR: Macros with complex values should be enclosed in parentheses
> 
> Here we define a constant that can be used to initialize a structure.
> Adding parentheses would lead to a compile time error:
> error: braced-group within expression allowed only inside a function

>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  foo.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
>  create mode 100644 foo.h
> 
> diff --git a/foo.h b/foo.h
> new file mode 100644
> index 000000000000..e2cba533f065
> --- /dev/null
> +++ b/foo.h
> @@ -0,0 +1,8 @@
> +/* SPDX-License-Identifier: BSD-2 */
> +#define EFI_ST_DISK_IMG { 0x00003368, { \
> +	{0x00000d40, "\x6f\x63\x00\x2f\x2a\x00\x20\x2a"}, /* oc./*. * */ \
> +	{0x00000d48, "\x00\x20\x2a\x2f\x0a\x00\x09\x7b"}, /* . */...{ */ \

I think this line would not do what you expect as the "/* . */" is
a complete comment and the "...{ */" is parsed as code.

> +	{0x00000d50, "\x30\x78\x25\x30\x38\x7a\x78\x2c"}, /* 0x%08zx, */ \
> +	{0x00000d58, "\x20\x22\x00\x5c\x78\x25\x30\x32"}, /*  ".\x%02 */ \
> +	{0x00000d60, "\x78\x00\x20\x2a\x2f\x20\x5c\x00"}, /* x. */ \. */ \

here too...

> +	{0, NULL} } }

checkpatch will always be a stupid style checker.

Some false positives are expected.

I think this is one of them, but perhaps the bit of
checkpatch logic in the COMPLEX_MACRO test could be
improved by something like the below.

Andy?  What do you think?

---
 scripts/checkpatch.pl | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 24618dffc5cb..9d3bdab03225 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -785,6 +785,8 @@ our $Typecast	= qr{\s*(\(\s*$NonptrType\s*\)){0,1}\s*};
 # Any use must be runtime checked with $^V
 
 our $balanced_parens = qr/(\((?:[^\(\)]++|(?-1))*\))/;
+our $balanced_braces = qr/(\{(?:[^\{\}]++|(?-1))*\})/;
+our $balanced_brackets = qr/(\[(?:[^\[\]]++|(?-1))*\])/;
 our $LvalOrFunc	= qr{((?:[\&\*]\s*)?$Lval)\s*($balanced_parens{0,1})\s*};
 our $FuncArg = qr{$Typecast{0,1}($LvalOrFunc|$Constant|$String)};
 
@@ -4953,6 +4955,16 @@ sub process {
 			$dstat =~ s/^\s*//s;
 			$dstat =~ s/\s*$//s;
 
+			# Flatten any parentheses and braces using the
+			# fancy balanced_<foo> tests (perl v5.10+ only)
+			if ($^V && $^V ge 5.10.0) {
+				while ($dstat =~ s/$balanced_parens/1/ ||
+				       $dstat =~ s/$balanced_braces/1/ ||
+				       $dstat =~ s/$balanced_brackets/1/)
+				{
+				}
+			}
+
 			# Flatten any parentheses and braces
 			while ($dstat =~ s/\([^\(\)]*\)/1/ ||
 			       $dstat =~ s/\{[^\{\}]*\}/1/ ||

Re: [BUG] scripts/checkpatch.pl: false positive missing parentheses

[Heinrich Schuchardt]

On 05/07/2018 10:43 PM, Joe Perches wrote:
> On Mon, 2018-05-07 at 20:31 +0200, Heinrich Schuchardt wrote:
>> This patch creates a false positive:
>> ERROR: Macros with complex values should be enclosed in parentheses
>>
>> Here we define a constant that can be used to initialize a structure.
>> Adding parentheses would lead to a compile time error:
>> error: braced-group within expression allowed only inside a function
> 
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  foo.h | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>  create mode 100644 foo.h
>>
>> diff --git a/foo.h b/foo.h
>> new file mode 100644
>> index 000000000000..e2cba533f065
>> --- /dev/null
>> +++ b/foo.h
>> @@ -0,0 +1,8 @@
>> +/* SPDX-License-Identifier: BSD-2 */
>> +#define EFI_ST_DISK_IMG { 0x00003368, { \
>> +	{0x00000d40, "\x6f\x63\x00\x2f\x2a\x00\x20\x2a"}, /* oc./*. * */ \
>> +	{0x00000d48, "\x00\x20\x2a\x2f\x0a\x00\x09\x7b"}, /* . */...{ */ \
> 
> I think this line would not do what you expect as the "/* . */" is
> a complete comment and the "...{ */" is parsed as code.

Thanks for catching this. If I replace the "internal" */ by ?/ the
problem does not occur. So probably we can close this issue.

So I think we can close the issue.

Best regards

Heinrich

> 
>> +	{0x00000d50, "\x30\x78\x25\x30\x38\x7a\x78\x2c"}, /* 0x%08zx, */ \
>> +	{0x00000d58, "\x20\x22\x00\x5c\x78\x25\x30\x32"}, /*  ".\x%02 */ \
>> +	{0x00000d60, "\x78\x00\x20\x2a\x2f\x20\x5c\x00"}, /* x. */ \. */ \
> 
> here too...
> 
>> +	{0, NULL} } }
> 
> checkpatch will always be a stupid style checker.
> 
> Some false positives are expected.
> 
> I think this is one of them, but perhaps the bit of
> checkpatch logic in the COMPLEX_MACRO test could be
> improved by something like the below.

Do we have to consider something a macro with complex a

> 
> Andy?  What do you think?
> 
> ---
>  scripts/checkpatch.pl | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 24618dffc5cb..9d3bdab03225 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -785,6 +785,8 @@ our $Typecast	= qr{\s*(\(\s*$NonptrType\s*\)){0,1}\s*};
>  # Any use must be runtime checked with $^V
>  
>  our $balanced_parens = qr/(\((?:[^\(\)]++|(?-1))*\))/;
> +our $balanced_braces = qr/(\{(?:[^\{\}]++|(?-1))*\})/;
> +our $balanced_brackets = qr/(\[(?:[^\[\]]++|(?-1))*\])/;
>  our $LvalOrFunc	= qr{((?:[\&\*]\s*)?$Lval)\s*($balanced_parens{0,1})\s*};
>  our $FuncArg = qr{$Typecast{0,1}($LvalOrFunc|$Constant|$String)};
>  
> @@ -4953,6 +4955,16 @@ sub process {
>  			$dstat =~ s/^\s*//s;
>  			$dstat =~ s/\s*$//s;
>  
> +			# Flatten any parentheses and braces using the
> +			# fancy balanced_<foo> tests (perl v5.10+ only)
> +			if ($^V && $^V ge 5.10.0) {
> +				while ($dstat =~ s/$balanced_parens/1/ ||
> +				       $dstat =~ s/$balanced_braces/1/ ||
> +				       $dstat =~ s/$balanced_brackets/1/)
> +				{
> +				}
> +			}
> +
>  			# Flatten any parentheses and braces
>  			while ($dstat =~ s/\([^\(\)]*\)/1/ ||
>  			       $dstat =~ s/\{[^\{\}]*\}/1/ ||
> 
> 
>