[Cocci] [RFC] coccinelle: add style check for assignment in if

[Cocci] [RFC] coccinelle: add style check for assignment in if

[Kris Borer]

Add a semantic patch for fixing some cases of checkpatch.pl error:

ERROR: do not use assignment in if condition

Signed-off-by: Kris Borer <kborer@gmail.com>
---
 scripts/coccinelle/style/assignment_in_if.cocci | 82 +++++++++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 scripts/coccinelle/style/assignment_in_if.cocci

diff --git a/scripts/coccinelle/style/assignment_in_if.cocci b/scripts/coccinelle/style/assignment_in_if.cocci
new file mode 100644
index 0000000..d9895e7
--- /dev/null
+++ b/scripts/coccinelle/style/assignment_in_if.cocci
@@ -0,0 +1,82 @@
+// find checkpatch.pl errors of the type:
+//	ERROR: do not use assignment in if condition
+//
+// Confidence: Moderate
+
+
+// if ( ret = call() )
+ at if1@
+identifier i;
+expression E;
+statement S1, S2;
+@@
+
++ i = E;
+  if (
+- (i = E)
++ i
+  ) S1 else S2
+
+
+// if ( (ret = call()) < 0 )
+@if2@
+identifier i;
+expression E;
+statement S1, S2;
+binary operator b;
+@@
+
++ i = E;
+  if (
+- (i = E)
++ i
+  b ... ) S1 else S2
+
+// if ( ptr->fun && (ret = ptr->fun()) < 0 )
+@if3@
+identifier i, i2;
+expression E1, E2;
+constant c;
+binary operator b;
+@@
+
++ if( E1->i ) {
++  	i2 = E2;
++ 	if (i2 < c) {
+- if( E1->i && ((i2 = E2) b c) ) {
+  ...
+- }
++ 	}
++ }
+
+// if ( (ret = call()) < 0 && ret != 0 )
+ at if4@
+identifier i;
+expression E, E2, E3;
+statement S1, S2;
+binary operator b;
+@@
+
++ i = E;
+  if (
+- (i = E)
++ i
+  b
+  ... && E2 && E3 ) S1 else S2
+
+// if ( (ret = call()) < 0 && ret != 0 && ret != 0 )
+@if5@
+identifier i;
+expression E, E2, E3;
+statement S1, S2;
+binary operator b;
+@@
+
++ i = E;
+  if (
+- (i = E)
++ i
+  b
+  ... && E2 && E3 ) S1 else S2
+
+
-- 
1.9.1

[RFC] coccinelle: add style check for assignment in if

[Kris Borer]

Add a semantic patch for fixing some cases of checkpatch.pl error:

ERROR: do not use assignment in if condition

Signed-off-by: Kris Borer <kborer@gmail.com>
---
 scripts/coccinelle/style/assignment_in_if.cocci | 82 +++++++++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 scripts/coccinelle/style/assignment_in_if.cocci

diff --git a/scripts/coccinelle/style/assignment_in_if.cocci b/scripts/coccinelle/style/assignment_in_if.cocci
new file mode 100644
index 0000000..d9895e7
--- /dev/null
+++ b/scripts/coccinelle/style/assignment_in_if.cocci
@@ -0,0 +1,82 @@
+// find checkpatch.pl errors of the type:
+//	ERROR: do not use assignment in if condition
+//
+// Confidence: Moderate
+
+
+// if ( ret = call() )
+@if1@
+identifier i;
+expression E;
+statement S1, S2;
+@@
+
++ i = E;
+  if (
+- (i = E)
++ i
+  ) S1 else S2
+
+
+// if ( (ret = call()) < 0 )
+@if2@
+identifier i;
+expression E;
+statement S1, S2;
+binary operator b;
+@@
+
++ i = E;
+  if (
+- (i = E)
++ i
+  b ... ) S1 else S2
+
+// if ( ptr->fun && (ret = ptr->fun()) < 0 )
+@if3@
+identifier i, i2;
+expression E1, E2;
+constant c;
+binary operator b;
+@@
+
++ if( E1->i ) {
++  	i2 = E2;
++ 	if (i2 < c) {
+- if( E1->i && ((i2 = E2) b c) ) {
+  ...
+- }
++ 	}
++ }
+
+// if ( (ret = call()) < 0 && ret != 0 )
+@if4@
+identifier i;
+expression E, E2, E3;
+statement S1, S2;
+binary operator b;
+@@
+
++ i = E;
+  if (
+- (i = E)
++ i
+  b
+  ... && E2 && E3 ) S1 else S2
+
+// if ( (ret = call()) < 0 && ret != 0 && ret != 0 )
+@if5@
+identifier i;
+expression E, E2, E3;
+statement S1, S2;
+binary operator b;
+@@
+
++ i = E;
+  if (
+- (i = E)
++ i
+  b
+  ... && E2 && E3 ) S1 else S2
+
+
-- 
1.9.1

[Cocci] [RFC] coccinelle: add style check for assignment in if

[Julia Lawall]

Thanks for the contribution.  Have you checked very carefully that it
doesn't eg pull XXX out of if (E && XXX)?  (I don't know if it does or it
doesn't, but it is a common pitfall with this issue).

thanks,
julia

On Wed, 12 Aug 2015, Kris Borer wrote:

> Add a semantic patch for fixing some cases of checkpatch.pl error:
>
> ERROR: do not use assignment in if condition
>
> Signed-off-by: Kris Borer <kborer@gmail.com>
> ---
>  scripts/coccinelle/style/assignment_in_if.cocci | 82 +++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 scripts/coccinelle/style/assignment_in_if.cocci
>
> diff --git a/scripts/coccinelle/style/assignment_in_if.cocci b/scripts/coccinelle/style/assignment_in_if.cocci
> new file mode 100644
> index 0000000..d9895e7
> --- /dev/null
> +++ b/scripts/coccinelle/style/assignment_in_if.cocci
> @@ -0,0 +1,82 @@
> +// find checkpatch.pl errors of the type:
> +//	ERROR: do not use assignment in if condition
> +//
> +// Confidence: Moderate
> +
> +
> +// if ( ret = call() )
> + at if1@
> +identifier i;
> +expression E;
> +statement S1, S2;
> +@@
> +
> ++ i = E;
> +  if (
> +- (i = E)
> ++ i
> +  ) S1 else S2
> +
> +
> +// if ( (ret = call()) < 0 )
> + at if2@
> +identifier i;
> +expression E;
> +statement S1, S2;
> +binary operator b;
> +@@
> +
> ++ i = E;
> +  if (
> +- (i = E)
> ++ i
> +  b ... ) S1 else S2
> +
> +// if ( ptr->fun && (ret = ptr->fun()) < 0 )
> + at if3@
> +identifier i, i2;
> +expression E1, E2;
> +constant c;
> +binary operator b;
> +@@
> +
> ++ if( E1->i ) {
> ++  	i2 = E2;
> ++ 	if (i2 < c) {
> +- if( E1->i && ((i2 = E2) b c) ) {
> +  ...
> +- }
> ++ 	}
> ++ }
> +
> +// if ( (ret = call()) < 0 && ret != 0 )
> + at if4@
> +identifier i;
> +expression E, E2, E3;
> +statement S1, S2;
> +binary operator b;
> +@@
> +
> ++ i = E;
> +  if (
> +- (i = E)
> ++ i
> +  b
> +  ... && E2 && E3 ) S1 else S2
> +
> +// if ( (ret = call()) < 0 && ret != 0 && ret != 0 )
> + at if5@
> +identifier i;
> +expression E, E2, E3;
> +statement S1, S2;
> +binary operator b;
> +@@
> +
> ++ i = E;
> +  if (
> +- (i = E)
> ++ i
> +  b
> +  ... && E2 && E3 ) S1 else S2
> +
> +
> --
> 1.9.1
>
>

Re: [RFC] coccinelle: add style check for assignment in if

[Julia Lawall]

Thanks for the contribution.  Have you checked very carefully that it
doesn't eg pull XXX out of if (E && XXX)?  (I don't know if it does or it
doesn't, but it is a common pitfall with this issue).

thanks,
julia

On Wed, 12 Aug 2015, Kris Borer wrote:

> Add a semantic patch for fixing some cases of checkpatch.pl error:
>
> ERROR: do not use assignment in if condition
>
> Signed-off-by: Kris Borer <kborer@gmail.com>
> ---
>  scripts/coccinelle/style/assignment_in_if.cocci | 82 +++++++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 scripts/coccinelle/style/assignment_in_if.cocci
>
> diff --git a/scripts/coccinelle/style/assignment_in_if.cocci b/scripts/coccinelle/style/assignment_in_if.cocci
> new file mode 100644
> index 0000000..d9895e7
> --- /dev/null
> +++ b/scripts/coccinelle/style/assignment_in_if.cocci
> @@ -0,0 +1,82 @@
> +// find checkpatch.pl errors of the type:
> +//	ERROR: do not use assignment in if condition
> +//
> +// Confidence: Moderate
> +
> +
> +// if ( ret = call() )
> +@if1@
> +identifier i;
> +expression E;
> +statement S1, S2;
> +@@
> +
> ++ i = E;
> +  if (
> +- (i = E)
> ++ i
> +  ) S1 else S2
> +
> +
> +// if ( (ret = call()) < 0 )
> +@if2@
> +identifier i;
> +expression E;
> +statement S1, S2;
> +binary operator b;
> +@@
> +
> ++ i = E;
> +  if (
> +- (i = E)
> ++ i
> +  b ... ) S1 else S2
> +
> +// if ( ptr->fun && (ret = ptr->fun()) < 0 )
> +@if3@
> +identifier i, i2;
> +expression E1, E2;
> +constant c;
> +binary operator b;
> +@@
> +
> ++ if( E1->i ) {
> ++  	i2 = E2;
> ++ 	if (i2 < c) {
> +- if( E1->i && ((i2 = E2) b c) ) {
> +  ...
> +- }
> ++ 	}
> ++ }
> +
> +// if ( (ret = call()) < 0 && ret != 0 )
> +@if4@
> +identifier i;
> +expression E, E2, E3;
> +statement S1, S2;
> +binary operator b;
> +@@
> +
> ++ i = E;
> +  if (
> +- (i = E)
> ++ i
> +  b
> +  ... && E2 && E3 ) S1 else S2
> +
> +// if ( (ret = call()) < 0 && ret != 0 && ret != 0 )
> +@if5@
> +identifier i;
> +expression E, E2, E3;
> +statement S1, S2;
> +binary operator b;
> +@@
> +
> ++ i = E;
> +  if (
> +- (i = E)
> ++ i
> +  b
> +  ... && E2 && E3 ) S1 else S2
> +
> +
> --
> 1.9.1
>
>

[Cocci] [RFC] coccinelle: add style check for assignment in if

[Kris Borer]

On Wed, Aug 12, 2015 at 10:00 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:

> Thanks for the contribution.  Have you checked very carefully that it
> doesn't eg pull XXX out of if (E && XXX)?  (I don't know if it does or it
> doesn't, but it is a common pitfall with this issue).
>
> thanks,
> julia
>

?I tested it and it seems okay. The details are below. Let me know if you
meant something else or would like a more involved test.

?bash> cat test.c
int f() {
    return 0;
}

int main() {

    int a;

    // if1 good
    if( a = f() ) {
        a = 1;
    }

    // if1 bad
    if( a == 1 && (a = f()) ) {
        a = 1;
    }

    // if2 good
    if( (a = f()) < 0 ) {
        a = 1;
    }

    // if2 bad
    if( a == 1 && (a = f()) < 0 ) {
        a = 1;
    }

    // if4 good
    if( (a = f()) < 0 && a != 0 ) {
        a = 1;
    }
    // if4 bad
    if( a == 1 && (a = f()) < 0 && a != 0 ) {
        a = 1;
    }

    // if5 good
    if( (a = f()) < 0 && a != 0 ) {
        a = 1;
    }
    // if5 bad
    if( a == 1 && (a = f()) < 0 && a != 0 ) {
        a = 1;
    }

    return 0;
}
bash> spatch --sp-file assignment_in_if.cocci test.c
init_defs_builtins: /usr/local/lib/coccinelle/standard.h
warning: if4: metavariable E3 not used in the - or context code
HANDLING: test.c
diff =
--- test.c
+++ /tmp/cocci-output-11515-32ee81-test.c
@@ -7,7 +7,8 @@ int main() {
     int a;

     // if1 good
-    if( a = f() ) {
+    a = f();
+    if(a) {
         a = 1;
     }

@@ -17,7 +18,8 @@ int main() {
     }

     // if2 good
-    if( (a = f()) < 0 ) {
+    a = f();
+    if(a < 0 ) {
         a = 1;
     }

@@ -27,7 +29,8 @@ int main() {
     }

     // if4 good
-    if( (a = f()) < 0 && a != 0 ) {
+    a = f();
+    if(a < 0 && a != 0 ) {
         a = 1;
     }
     // if4 bad
@@ -36,7 +39,8 @@ int main() {
     }

     // if5 good
-    if( (a = f()) < 0 && a != 0 ) {
+    a = f();
+    if(a < 0 && a != 0 ) {
         a = 1;
     }
     // if5 bad
?
Thanks,

?Kris Borer?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20150812/b53e6227/attachment.html>

[Cocci] [RFC] coccinelle: add style check for assignment in if

[Michal Marek]

On 2015-08-12 15:51, Kris Borer wrote:
> Add a semantic patch for fixing some cases of checkpatch.pl error:
> 
> ERROR: do not use assignment in if condition

There is a gcc warning for this already.

Michal

Re: [RFC] coccinelle: add style check for assignment in if

[Michal Marek]

On 2015-08-12 15:51, Kris Borer wrote:
> Add a semantic patch for fixing some cases of checkpatch.pl error:
> 
> ERROR: do not use assignment in if condition

There is a gcc warning for this already.

Michal

[Cocci] [RFC] coccinelle: add style check for assignment in if

[Kris Borer]

On Wed, Aug 12, 2015 at 10:12 AM, Michal Marek <mmarek@suse.cz> wrote:

> On 2015-08-12 15:51, Kris Borer wrote:
> > Add a semantic patch for fixing some cases of checkpatch.pl error:
> >
> > ERROR: do not use assignment in if condition
>
> There is a gcc warning for this already.
>
> Michal
>
>
?My intention was not to create another way to uncover problems but rather
to ?provide a tool for people to use to fix them. Let me know if I am
misunderstanding the purpose of this subsystem.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20150812/cc990076/attachment.html>

[Cocci] [RFC] coccinelle: add style check for assignment in if

[Michal Marek]

On 2015-08-12 16:53, Kris Borer wrote:
> On Wed, Aug 12, 2015 at 10:12 AM, Michal Marek <mmarek@suse.cz
> <mailto:mmarek@suse.cz>> wrote:
> 
>     On 2015-08-12 15:51, Kris Borer wrote:
>     > Add a semantic patch for fixing some cases of checkpatch.pl <http://checkpatch.pl> error:
>     >
>     > ERROR: do not use assignment in if condition
> 
>     There is a gcc warning for this already.
> 
>     Michal
> 
> 
> ?My intention was not to create another way to uncover problems but
> rather to ?provide a tool for people to use to fix them. Let me know if
> I am misunderstanding the purpose of this subsystem.

OK, so this is fixing a style issue, and not cases of accidental
assignment instead of '==' (for which there is a gcc warning and we
hopefully do not have such errors in the kernel). While I'm probably
ignorant and no not see how one style is better than the other, I see
that some maintainers already applied your patches based on this check.
So I'll merge it once Julia acks it.

P.S.: Please switch of HTML email, otherwise vger.kernel.org won't
accept your messages.

Michal

Re: [RFC] coccinelle: add style check for assignment in if

[Michal Marek]

On 2015-08-12 16:53, Kris Borer wrote:
> On Wed, Aug 12, 2015 at 10:12 AM, Michal Marek <mmarek@suse.cz
> <mailto:mmarek@suse.cz>> wrote:
> 
>     On 2015-08-12 15:51, Kris Borer wrote:
>     > Add a semantic patch for fixing some cases of checkpatch.pl <http://checkpatch.pl> error:
>     >
>     > ERROR: do not use assignment in if condition
> 
>     There is a gcc warning for this already.
> 
>     Michal
> 
> 
> ​My intention was not to create another way to uncover problems but
> rather to ​provide a tool for people to use to fix them. Let me know if
> I am misunderstanding the purpose of this subsystem.

OK, so this is fixing a style issue, and not cases of accidental
assignment instead of '==' (for which there is a gcc warning and we
hopefully do not have such errors in the kernel). While I'm probably
ignorant and no not see how one style is better than the other, I see
that some maintainers already applied your patches based on this check.
So I'll merge it once Julia acks it.

P.S.: Please switch of HTML email, otherwise vger.kernel.org won't
accept your messages.

Michal

[Cocci] [RFC] coccinelle: add style check for assignment in if

[Julia Lawall]

On Wed, 12 Aug 2015, Michal Marek wrote:

> On 2015-08-12 16:53, Kris Borer wrote:
> > On Wed, Aug 12, 2015 at 10:12 AM, Michal Marek <mmarek@suse.cz
> > <mailto:mmarek@suse.cz>> wrote:
> >
> >     On 2015-08-12 15:51, Kris Borer wrote:
> >     > Add a semantic patch for fixing some cases of checkpatch.pl <http://checkpatch.pl> error:
> >     >
> >     > ERROR: do not use assignment in if condition
> >
> >     There is a gcc warning for this already.
> >
> >     Michal
> >
> >
> > ?My intention was not to create another way to uncover problems but
> > rather to ?provide a tool for people to use to fix them. Let me know if
> > I am misunderstanding the purpose of this subsystem.
>
> OK, so this is fixing a style issue, and not cases of accidental
> assignment instead of '==' (for which there is a gcc warning and we
> hopefully do not have such errors in the kernel). While I'm probably
> ignorant and no not see how one style is better than the other, I see
> that some maintainers already applied your patches based on this check.
> So I'll merge it once Julia acks it.

Actually, assignments inside if tests are really annoying for Coccinelle,
because there become two different control flows from the assignment to
the test on the result.  So I would be happy to see these go away.

I'll check the semantic patch as soon as possible.

julia

Re: [RFC] coccinelle: add style check for assignment in if

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1360 bytes --]



On Wed, 12 Aug 2015, Michal Marek wrote:

> On 2015-08-12 16:53, Kris Borer wrote:
> > On Wed, Aug 12, 2015 at 10:12 AM, Michal Marek <mmarek@suse.cz
> > <mailto:mmarek@suse.cz>> wrote:
> >
> >     On 2015-08-12 15:51, Kris Borer wrote:
> >     > Add a semantic patch for fixing some cases of checkpatch.pl <http://checkpatch.pl> error:
> >     >
> >     > ERROR: do not use assignment in if condition
> >
> >     There is a gcc warning for this already.
> >
> >     Michal
> >
> >
> > ​My intention was not to create another way to uncover problems but
> > rather to ​provide a tool for people to use to fix them. Let me know if
> > I am misunderstanding the purpose of this subsystem.
>
> OK, so this is fixing a style issue, and not cases of accidental
> assignment instead of '==' (for which there is a gcc warning and we
> hopefully do not have such errors in the kernel). While I'm probably
> ignorant and no not see how one style is better than the other, I see
> that some maintainers already applied your patches based on this check.
> So I'll merge it once Julia acks it.

Actually, assignments inside if tests are really annoying for Coccinelle,
because there become two different control flows from the assignment to
the test on the result.  So I would be happy to see these go away.

I'll check the semantic patch as soon as possible.

julia