[cocci] Checking a transformation approach for the avoidance of duplicate code

[cocci] Checking a transformation approach for the avoidance of duplicate code

[Markus Elfring]

Hello,

The analysis tool “Cppcheck 2.10.3-3.2” pointed a source code place out
for further development considerations.

https://github.com/fvwmorg/fvwm3/blob/c9d00184ac3ab534e0f0ba5683106879a35857f6/fvwm/colorset.c#L965

(style) Condition 'tint==NULL' is always true [knownConditionTrueFalse]

See also a change suggestion:
Simplify data processing for tint colour in parse_colorset()
https://github.com/fvwmorg/fvwm3/issues/897


I constructed another SmPL script variant for a desirable transformation.


@replacement@
statement s1, s2, s3;
@@
-if (tint != NULL)
-{
- s1
- s2
- cs->tint = GetColor(tint);
- s3
-}
-else if (tint == NULL)
-{
- s1
- s2
- cs->tint = GetColor(black);
- s3
-}
+s1
+s2
+cs->tint = GetColor(tint ? tint : black);
+s3


Now I wonder why a corresponding patch is not generated by the software
“Coccinelle 1.1.1-00478-g0afff7fa” (OCaml 5.0) at the moment.
I would appreciate further advices.

Regards,
Markus

Re: [cocci] Checking a transformation approach for the avoidance of duplicate code

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1137 bytes --]



On Sat, 2 Sep 2023, Markus Elfring wrote:

> Hello,
>
> The analysis tool “Cppcheck 2.10.3-3.2” pointed a source code place out
> for further development considerations.
>
> https://github.com/fvwmorg/fvwm3/blob/c9d00184ac3ab534e0f0ba5683106879a35857f6/fvwm/colorset.c#L965
>
> (style) Condition 'tint==NULL' is always true [knownConditionTrueFalse]
>
> See also a change suggestion:
> Simplify data processing for tint colour in parse_colorset()
> https://github.com/fvwmorg/fvwm3/issues/897
>
>
> I constructed another SmPL script variant for a desirable transformation.
>
>
> @replacement@
> statement s1, s2, s3;
> @@
> -if (tint != NULL)
> -{
> - s1
> - s2
> - cs->tint = GetColor(tint);
> - s3
> -}
> -else if (tint == NULL)
> -{
> - s1
> - s2
> - cs->tint = GetColor(black);
> - s3
> -}
> +s1
> +s2
> +cs->tint = GetColor(tint ? tint : black);
> +s3
>
>
> Now I wonder why a corresponding patch is not generated by the software
> “Coccinelle 1.1.1-00478-g0afff7fa” (OCaml 5.0) at the moment.
> I would appreciate further advices.

Probably because in each branch, the first line is a declaration, not a
statement.

julia

Re: [cocci] Checking a transformation approach for the avoidance of duplicate code

[Markus Elfring]

>> I constructed another SmPL script variant for a desirable transformation.
>>
>>
>> @replacement@
>> statement s1, s2, s3;
>> @@
>> -if (tint != NULL)
>> -{
>> - s1
>> - s2
>> - cs->tint = GetColor(tint);
>> - s3
>> -}
>> -else if (tint == NULL)
>> -{
>> - s1
>> - s2
>> - cs->tint = GetColor(black);
>> - s3
>> -}
>> +s1
>> +s2
>> +cs->tint = GetColor(tint ? tint : black);
>> +s3
>>
>>
>> Now I wonder why a corresponding patch is not generated by the software
>> “Coccinelle 1.1.1-00478-g0afff7fa” (OCaml 5.0) at the moment.
>> I would appreciate further advices.
>
> Probably because in each branch, the first line is a declaration, not a statement.

How does this feedback fit to other software documentation according to declaration statements?
https://en.cppreference.com/w/cpp/language/statements#Declaration_statements

Regards,
Markus

Re: [cocci] Checking a transformation approach for the avoidance of duplicate code

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1121 bytes --]



On Sat, 2 Sep 2023, Markus Elfring wrote:

> >> I constructed another SmPL script variant for a desirable transformation.
> >>
> >>
> >> @replacement@
> >> statement s1, s2, s3;
> >> @@
> >> -if (tint != NULL)
> >> -{
> >> - s1
> >> - s2
> >> - cs->tint = GetColor(tint);
> >> - s3
> >> -}
> >> -else if (tint == NULL)
> >> -{
> >> - s1
> >> - s2
> >> - cs->tint = GetColor(black);
> >> - s3
> >> -}
> >> +s1
> >> +s2
> >> +cs->tint = GetColor(tint ? tint : black);
> >> +s3
> >>
> >>
> >> Now I wonder why a corresponding patch is not generated by the software
> >> “Coccinelle 1.1.1-00478-g0afff7fa” (OCaml 5.0) at the moment.
> >> I would appreciate further advices.
> >
> > Probably because in each branch, the first line is a declaration, not a statement.
>
> How does this feedback fit to other software documentation according to declaration statements?
> https://en.cppreference.com/w/cpp/language/statements#Declaration_statements

Whatever.  In Coccinelle, we decided to do it that way I said it is done.
This makes it easy to search for the first statement that is not a
declaration in a function.

julia

Re: [cocci] Checking a transformation approach for the avoidance of duplicate code

[Markus Elfring]

>>>> Now I wonder why a corresponding patch is not generated by the software
>>>> “Coccinelle 1.1.1-00478-g0afff7fa” (OCaml 5.0) at the moment.
>>>> I would appreciate further advices.
>>>
>>> Probably because in each branch, the first line is a declaration, not a statement.
>>
>> How does this feedback fit to other software documentation according to declaration statements?
>> https://en.cppreference.com/w/cpp/language/statements#Declaration_statements
>
> Whatever.  In Coccinelle, we decided to do it that way I said it is done.
> This makes it easy to search for the first statement that is not a
> declaration in a function.

The following SmPL script variant does also not produce an expected patch
for the source file “fvwm/colorset.c”.

@replacement@
declaration d;
statement s1, s2;
@@
-if (tint != NULL)
-{
- d
- s1
- cs->tint = GetColor(tint);
- s2
-}
-else if (tint == NULL)
-{
- d
- s1
- cs->tint = GetColor(black);
- s2
-}
+d
+s1
+cs->tint = GetColor(tint ? tint : black);
+s2


Regards,
Markus

Re: [cocci] Checking a transformation approach for the avoidance of duplicate code

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1216 bytes --]



On Sat, 2 Sep 2023, Markus Elfring wrote:

> >>>> Now I wonder why a corresponding patch is not generated by the software
> >>>> “Coccinelle 1.1.1-00478-g0afff7fa” (OCaml 5.0) at the moment.
> >>>> I would appreciate further advices.
> >>>
> >>> Probably because in each branch, the first line is a declaration, not a statement.
> >>
> >> How does this feedback fit to other software documentation according to declaration statements?
> >> https://en.cppreference.com/w/cpp/language/statements#Declaration_statements
> >
> > Whatever.  In Coccinelle, we decided to do it that way I said it is done.
> > This makes it easy to search for the first statement that is not a
> > declaration in a function.
>
> The following SmPL script variant does also not produce an expected patch
> for the source file “fvwm/colorset.c”.
>
> @replacement@
> declaration d;
> statement s1, s2;
> @@
> -if (tint != NULL)
> -{
> - d
> - s1
> - cs->tint = GetColor(tint);
> - s2
> -}
> -else if (tint == NULL)
> -{
> - d
> - s1
> - cs->tint = GetColor(black);
> - s2
> -}
> +d
> +s1
> +cs->tint = GetColor(tint ? tint : black);
> +s2

OK the problem seems to be related to s2 matching an if.  I will look into
it.

thanks,
julia

Re: [cocci] Checking a transformation approach for the avoidance of duplicate code

[Markus Elfring]

>> The following SmPL script variant does also not produce an expected patch
>> for the source file “fvwm/colorset.c”.
>>
>> @replacement@
>> declaration d;
>> statement s1, s2;
>> @@
>> -if (tint != NULL)
>> -{
>> - d
>> - s1
>> - cs->tint = GetColor(tint);
>> - s2
>> -}
>> -else if (tint == NULL)
>> -{
>> - d
>> - s1
>> - cs->tint = GetColor(black);
>> - s2
>> -}
>> +d
>> +s1
>> +cs->tint = GetColor(tint ? tint : black);
>> +s2
>
> OK the problem seems to be related to s2 matching an if.  I will look into it.

I tried another SmPL script variant out.

@replacement@
statement s;
@@
-if (tint == NULL)
-{
- ...
- cs->tint = GetColor(black);
- s
-}
+cs->tint = GetColor(tint ? tint : black);
+s


Markus_Elfring@Sonne:…/Projekte/fvwm3> spatch test_tint_determination2.cocci lokal/fvwm/colorset.c
…
@@ -962,14 +962,9 @@ void parse_colorset(int n, char *line)
                                have_pixels_changed = True;
                        }
                }
-               else if (tint == NULL)
-               {
-                       /* default */
-                       Pixel old_tint = cs->tint;
-                       PictureFreeColors(dpy, Pcmap, &cs->tint, 1, 0, True);
-                       cs->tint = GetColor(black);
-                       if (old_tint != cs->tint)
-                       {
+               else {
+                       cs->tint = GetColor(tint ? tint : black);
+                       if (old_tint != cs->tint) {
                                have_pixels_changed = True;
                        }
                }


Do you get any further development ideas from such a test result?

Regards,
Markus

Re: [cocci] Checking a transformation approach for the avoidance of duplicate code

[Markus Elfring]

> OK the problem seems to be related to s2 matching an if.  I will look into it.

How will the chances evolve for the clarification of the test result
that the following SmPL script variant does also not find known source code.

https://github.com/fvwmorg/fvwm3/blob/c9d00184ac3ab534e0f0ba5683106879a35857f6/fvwm/colorset.c#L955-L975

@display@
declaration d;
statement s1, s2;
@@
*if (tint != NULL)
*{
* d
* s1
* cs->tint = GetColor(tint);
* s2
*}
*else if (tint == NULL)
*{
* d
* s1
* cs->tint = GetColor(black);
* s2
*}


Would you like to point any debug approaches out for the observed behaviour
of the current Coccinelle software?

Regards,
Markus

Re: [cocci] Checking an analysis approach for the avoidance of duplicate code

[Markus Elfring]

> How will the chances evolve for the clarification of the test result
> that the following SmPL script variant does also not find known source code?
>
> https://github.com/fvwmorg/fvwm3/blob/c9d00184ac3ab534e0f0ba5683106879a35857f6/fvwm/colorset.c#L955-L975

@display@
statement s1, s2;
@@
*if (tint != NULL)
*{
* ...
* s1
* cs->tint = GetColor(tint);
* s2
*}
*else if (tint == NULL)
*{
* ...
* s1
* cs->tint = GetColor(black);
* s2
*}


Will any case distinctions be adjusted in the implementation of the Coccinelle software?

Regards,
Markus

Re: [cocci] Checking an analysis approach for the avoidance of duplicate code

[Markus Elfring]

> Will any case distinctions be adjusted in the implementation of the Coccinelle software?

Would you like to discuss data processing consequences any more for the use case
if multiple metavariables are specified with the same type?

Are there any special constraints to consider for the safe application
of the semantic patch language if a type like “statement” was chosen for some
variable names?

Regards,
Markus

Re: [cocci] Checking an analysis approach for the avoidance of duplicate code

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 623 bytes --]



On Wed, 6 Sep 2023, Markus Elfring wrote:

> > Will any case distinctions be adjusted in the implementation of the Coccinelle software?
>
> Would you like to discuss data processing consequences any more for the use case
> if multiple metavariables are specified with the same type?
>
> Are there any special constraints to consider for the safe application
> of the semantic patch language if a type like “statement” was chosen for some
> variable names?

None of the above questions make any sense to me, but I did spend some
time yesterday looking at the problem, and I don't have a solution for the
moment.

julia

Re: [cocci] Checking an analysis approach for the avoidance of duplicate code

[Markus Elfring]

>>> Will any case distinctions be adjusted in the implementation of the Coccinelle software?
>>
>> Would you like to discuss data processing consequences any more for the use case
>> if multiple metavariables are specified with the same type?
>>
>> Are there any special constraints to consider for the safe application
>> of the semantic patch language if a type like “statement” was chosen for some
>> variable names?
>
> None of the above questions make any sense to me,

I hope that such communication difficulties can be resolved better.


> but I did spend some time yesterday looking at the problem,

Thanks for this information.

Which impression did you get from the software situation?


> and I don't have a solution for the moment.

Will any clarifications become more helpful here?

Regards,
Markus