Re: [Xen-devel] [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

2019-09-01 Thread Markus Elfring
> +# nested likely/unlikely calls
> +    if ($line =~ 
> /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
> +    WARN("LIKELY_MISUSE",
…
>> \b(?:un)?likely\s*
>
> This pair of brackets is required to match "unlikely"
> and it's optional in order to match "likely".

I agree also to this view if you refer to the shortened regular expression here.
But I got an other development opinion for an extra pair of non-capturing 
parentheses
at the front (from the version which you suggested).

Regards,
Markus

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

2019-09-01 Thread Markus Elfring

+# nested likely/unlikely calls
+   if ($line =~ 
/\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
+   WARN("LIKELY_MISUSE",


How do you think about to use the specification 
“(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
in this regular expression?

Regards,
Markus

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

2019-09-01 Thread Markus Elfring
>>> +# nested likely/unlikely calls
>>> +    if ($line =~ 
>>> /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>> +    WARN("LIKELY_MISUSE",
>>
>> How do you think about to use the specification 
>> “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>> in this regular expression?
…
>IS_ERR
>(?:_ <- Another atomic group just to show that '_' is a common prefix?

Yes. - I hope that this specification detail can help a bit.


>Usually, Perl interpreter is very good at optimizing such things.

Would you like to help this software component by omitting a pair of
non-capturing parentheses at the beginning?

\b(?:un)?likely\s*


Regards,
Markus

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

2019-08-31 Thread Denis Efremov


On 31.08.2019 19:45, Markus Elfring wrote:
 +# nested likely/unlikely calls
 +    if ($line =~ 
 /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
 +    WARN("LIKELY_MISUSE",
>>>
>>> How do you think about to use the specification 
>>> “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>>> in this regular expression?
> …
>>IS_ERR
>>(?:_ <- Another atomic group just to show that '_' is a common prefix?
> 
> Yes. - I hope that this specification detail can help a bit.

I'm not sure that another pair of brackets for a single char worth it.

>>Usually, Perl interpreter is very good at optimizing such things.

The interpreter optimizes it internally:
echo 'IS_ERR_OR_NULL' | perl -Mre=debug -ne '/IS_ERR(?:_OR_NULL|_VALUE)?/ && 
print'
Compiling REx "IS_ERR(?:_OR_NULL|_VALUE)?"
Final program:
   1: EXACT  (4)
   4: CURLYX[0]{0,1} (16)
   6:   EXACT <_> (8)  <-- common prefix
   8:   TRIE-EXACT[OV] (15)
 

...
> 
> Would you like to help this software component by omitting a pair of
> non-capturing parentheses at the beginning?
> 
> \b(?:un)?likely\s*

This pair of brackets is required to match "unlikely" and it's
optional in order to match "likely".

Regards,
Denis

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

2019-08-31 Thread Denis Efremov


On 31.08.2019 12:15, Markus Elfring wrote:
>> +# nested likely/unlikely calls
>> +    if ($line =~ 
>> /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>> +    WARN("LIKELY_MISUSE",
> 
> How do you think about to use the specification 
> “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
> in this regular expression?

Hmm, 
(?:   <- Catch group is required here, since it is used in diagnostic message,
 see $1
   IS_ERR
   (?:_ <- Another atomic group just to show that '_' is a common prefix?
   I'm not sure about this. Usually, Perl interpreter is very good at 
optimizing such things.
   You could see this optimization if you run perl with -Mre=debug.
 (?:OR_NULL|VALUE))?|WARN)

Regards,
Denis

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v3 01/11] checkpatch: check for nested (un)?likely() calls

2019-08-29 Thread Denis Efremov
IS_ERR(), IS_ERR_OR_NULL(), IS_ERR_VALUE() and WARN*() already contain
unlikely() optimization internally. Thus, there is no point in calling
these functions and defines under likely()/unlikely().

This check is based on the coccinelle rule developed by Enrico Weigelt
https://lore.kernel.org/lkml/1559767582-11081-1-git-send-email-i...@metux.net/

Signed-off-by: Denis Efremov 
Cc: Joe Perches 
Cc: Andrew Morton 
Cc: Andy Whitcroft 
---
 scripts/checkpatch.pl | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 93a7edfe0f05..56969ce06df4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6480,6 +6480,12 @@ sub process {
 "Using $1 should generally have parentheses around 
the comparison\n" . $herecurr);
}
 
+# nested likely/unlikely calls
+   if ($line =~ 
/\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
+   WARN("LIKELY_MISUSE",
+"nested (un)?likely() calls, $1 already uses 
unlikely() internally\n" . $herecurr);
+   }
+
 # whine mightly about in_atomic
if ($line =~ /\bin_atomic\s*\(/) {
if ($realfile =~ m@^drivers/@) {
-- 
2.21.0


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel