[Cocci] Formatting issues

[Cocci] Formatting issues

[Daniel Wagner]

Hi,

I have found another small issue. Not a big thing. I have following rule

@@
identifier f =~ "^(__)?connman_.*" ;
@@

f(...,
(
- FALSE
+ false
|
- TRUE
+ true
)
 ,...)


And this little C example:

bool __connman_bar(bool baz);
int connman_foo(int val, bool bar);

int main(int argc, char *argv)
{
	int err;

	if (__connman_bar(FALSE) == TRUE)
		err = connman_a_rather_long_line(2434, TRUE);

	return 0;
}


Running coccinelle on this results in the not so nicely formated patch:

@@ -5,8 +5,9 @@ int main(int argc, char *argv)
 {
        int err;
 
-       if (__connman_bar(FALSE) == TRUE)
-               err = connman_a_rather_long_line(2434, TRUE);
+       if (__connman_bar(false) == TRUE)
+               err = connman_a_rather_long_line(2434,
+                                                                         true);
 
        return 0;
 }

When connman_a_rather_long_line() is not sooo long then it works as
expected, that means not additional line is introduced. Any ideas
what is going wrong?

cheers,
daniel

[Cocci] Formatting issues

[Julia Lawall]

On Wed, 17 Jul 2013, Daniel Wagner wrote:

> Hi,
>
> I have found another small issue. Not a big thing. I have following rule
>
> @@
> identifier f =~ "^(__)?connman_.*" ;
> @@
>
> f(...,
> (
> - FALSE
> + false
> |
> - TRUE
> + true
> )
> ,...)
>
>
> And this little C example:
>
> bool __connman_bar(bool baz);
> int connman_foo(int val, bool bar);
>
> int main(int argc, char *argv)
> {
> 	int err;
>
> 	if (__connman_bar(FALSE) == TRUE)
> 		err = connman_a_rather_long_line(2434, TRUE);
>
> 	return 0;
> }
>
>
> Running coccinelle on this results in the not so nicely formated patch:
>
> @@ -5,8 +5,9 @@ int main(int argc, char *argv)
> {
>        int err;
>
> -       if (__connman_bar(FALSE) == TRUE)
> -               err = connman_a_rather_long_line(2434, TRUE);
> +       if (__connman_bar(false) == TRUE)
> +               err = connman_a_rather_long_line(2434,
> +                                                                         true);
>
>        return 0;
> }
>
> When connman_a_rather_long_line() is not sooo long then it works as
> expected, that means not additional line is introduced. Any ideas
> what is going wrong?

I had this problem as well.  When the line is long, it is supposed to try 
to place the later arguments nicely.  But clearly something is going 
wrong, and I appreciate your report, because it shows that it is not just 
my code...

I will look at it in the next few days.

julia

[Cocci] Formatting issues

[Daniel Wagner]

On 07/17/2013 05:07 PM, Julia Lawall wrote:
> On Wed, 17 Jul 2013, Daniel Wagner wrote:
> 
>> Hi,
>>
>> I have found another small issue. Not a big thing. I have following rule
>>
>> @@
>> identifier f =~ "^(__)?connman_.*" ;
>> @@
>>
>> f(...,
>> (
>> - FALSE
>> + false
>> |
>> - TRUE
>> + true
>> )
>> ,...)
>>
>>
>> And this little C example:
>>
>> bool __connman_bar(bool baz);
>> int connman_foo(int val, bool bar);
>>
>> int main(int argc, char *argv)
>> {
>>     int err;
>>
>>     if (__connman_bar(FALSE) == TRUE)
>>         err = connman_a_rather_long_line(2434, TRUE);
>>
>>     return 0;
>> }
>>
>>
>> Running coccinelle on this results in the not so nicely formated patch:
>>
>> @@ -5,8 +5,9 @@ int main(int argc, char *argv)
>> {
>>        int err;
>>
>> -       if (__connman_bar(FALSE) == TRUE)
>> -               err = connman_a_rather_long_line(2434, TRUE);
>> +       if (__connman_bar(false) == TRUE)
>> +               err = connman_a_rather_long_line(2434,
>> +                                                                         
>> true);
>>
>>        return 0;
>> }
>>
>> When connman_a_rather_long_line() is not sooo long then it works as	
>> expected, that means not additional line is introduced. Any ideas
>> what is going wrong?
> 
> I had this problem as well.  When the line is long, it is supposed to 
> try to place the later arguments nicely.  But clearly something is going 
> wrong, and I appreciate your report, because it shows that it is not 
> just my code...
> 
> I will look at it in the next few days.

Thanks :)

Another formatting issue I found is triggered by a different rule. I figure
the expression 'E' is written out as one line and the original formatting gets
lost.

@@
expression E;
symbol TRUE;
symbol FALSE;
@@

(
- E == TRUE
+ E
|
- TRUE == E
+ E
|
- E != TRUE
+ !E
|
- TRUE != E
+ !E
|
- E == FALSE
+ !E
|
- FALSE == E
+ !E
|
- E != FALSE
+ E
|
- FALSE != E
+ E
)


the C code:


#define AGENT_INTERFACE ""

int main(int argc, char *argv[])
{
	if (g_dbus_register_interface(connection, path,
					AGENT_INTERFACE, agent_methods,
					NULL, NULL, &agent_request,
					NULL) == FALSE) {
		fprintf(stderr, "Error: Failed to register Agent callbacks\n");
                return 0;
        }

	return 0;
}


gives me this here:


int main(int argc, char *argv[])
 {
-       if (g_dbus_register_interface(connection, path,
-                                       AGENT_INTERFACE, agent_methods,
-                                       NULL, NULL, &agent_request,
-                                       NULL) == FALSE) {
+       if (!g_dbus_register_interface(connection, path, AGENT_INTERFACE, agent_methods, NULL, NULL, &agent_request, NULL)) {
                fprintf(stderr, "Error: Failed to register Agent callbacks\n");
                 return 0;
         }

cheers,
daniel

[Cocci] Formatting issues

[Julia Lawall]

> Another formatting issue I found is triggered by a different rule. I figure
> the expression 'E' is written out as one line and the original formatting gets
> lost.

Yes, there is not much I can do about this one.  When you match code into 
a metavariable, all of the whitespace and comments go away.  It doesn't 
know that you still want them in the place where you add the metavariable 
back in.

If you have a probleme with this, as in your example, you can try to 
rewrite the semantic patch so that the metavariable is there but does not 
appear in the - and + code.  For example, for your first case below you 
can write

E
- == TRUE

julia

> @@
> expression E;
> symbol TRUE;
> symbol FALSE;
> @@
>
> (
> - E == TRUE
> + E
> |
> - TRUE == E
> + E
> |
> - E != TRUE
> + !E
> |
> - TRUE != E
> + !E
> |
> - E == FALSE
> + !E
> |
> - FALSE == E
> + !E
> |
> - E != FALSE
> + E
> |
> - FALSE != E
> + E
> )
>
>
> the C code:
>
>
> #define AGENT_INTERFACE ""
>
> int main(int argc, char *argv[])
> {
> 	if (g_dbus_register_interface(connection, path,
> 					AGENT_INTERFACE, agent_methods,
> 					NULL, NULL, &agent_request,
> 					NULL) == FALSE) {
> 		fprintf(stderr, "Error: Failed to register Agent callbacks\n");
>                return 0;
>        }
>
> 	return 0;
> }
>
>
> gives me this here:
>
>
> int main(int argc, char *argv[])
> {
> -       if (g_dbus_register_interface(connection, path,
> -                                       AGENT_INTERFACE, agent_methods,
> -                                       NULL, NULL, &agent_request,
> -                                       NULL) == FALSE) {
> +       if (!g_dbus_register_interface(connection, path, AGENT_INTERFACE, agent_methods, NULL, NULL, &agent_request, NULL)) {
>                fprintf(stderr, "Error: Failed to register Agent callbacks\n");
>                 return 0;
>         }
>
> cheers,
> daniel
>
>

[Cocci] Formatting issues

[Daniel Wagner]

On 07/17/2013 05:31 PM, Julia Lawall wrote:
>> Another formatting issue I found is triggered by a different rule. I
>> figure
>> the expression 'E' is written out as one line and the original
>> formatting gets
>> lost.
>
> Yes, there is not much I can do about this one.  When you match code
> into a metavariable, all of the whitespace and comments go away.  It
> doesn't know that you still want them in the place where you add the
> metavariable back in.
>
> If you have a probleme with this, as in your example, you can try to
> rewrite the semantic patch so that the metavariable is there but does
> not appear in the - and + code.  For example, for your first case below
> you can write
>
> E
> - == TRUE

Well, it is there only around 20 places where the formatting gets 
corrupted via this rule. I'll just write a cleanup patch after
this automatic change. That is okay.

cheers,
daniel

[Cocci] Formatting issues

[Ondřej Bílka]

On Wed, Jul 17, 2013 at 05:31:15PM +0200, Julia Lawall wrote:
> >Another formatting issue I found is triggered by a different rule. I figure
> >the expression 'E' is written out as one line and the original formatting gets
> >lost.
> 
> Yes, there is not much I can do about this one.  When you match code
> into a metavariable, all of the whitespace and comments go away.  It
> doesn't know that you still want them in the place where you add the
> metavariable back in.
> 
> If you have a probleme with this, as in your example, you can try to
> rewrite the semantic patch so that the metavariable is there but
> does not appear in the - and + code.  For example, for your first
> case below you can write
> 
I ran to problems with formating when I tried use cocci in glibc.

I think that semantic patch and its formatting are two separate issues.

Ideal situation is when project formatting is fully machine specified.
Then we can apply patch, run formatter and we are done. 
Otherwise we can try to convince other developers that benefit of refactoring
is bigger than formatting issues it causes or fix it by hand.

I now try to convince others to use first option as this allows all
submitted patches have perfect formatting because formatter took care of
it. This can save few round trips when reviewer wants space after comma
etc.

Ondra

[Cocci] Formatting issues

[Daniel Wagner]

On 07/17/2013 05:51 PM, Ond?ej B?lka wrote:
> On Wed, Jul 17, 2013 at 05:31:15PM +0200, Julia Lawall wrote:
>>> Another formatting issue I found is triggered by a different rule. I figure
>>> the expression 'E' is written out as one line and the original formatting gets
>>> lost.
>>
>> Yes, there is not much I can do about this one.  When you match code
>> into a metavariable, all of the whitespace and comments go away.  It
>> doesn't know that you still want them in the place where you add the
>> metavariable back in.
>>
>> If you have a probleme with this, as in your example, you can try to
>> rewrite the semantic patch so that the metavariable is there but
>> does not appear in the - and + code.  For example, for your first
>> case below you can write
>>
> I ran to problems with formating when I tried use cocci in glibc.
>
> I think that semantic patch and its formatting are two separate issues.

Thought semantic patching can't just ignore formatting.

> Ideal situation is when project formatting is fully machine specified.
> Then we can apply patch, run formatter and we are done.

In my case, I can use checkpatch.pl to report some errors.

> Otherwise we can try to convince other developers that benefit of refactoring
> is bigger than formatting issues it causes or fix it by hand.
>
> I now try to convince others to use first option as this allows all
> submitted patches have perfect formatting because formatter took care of
> it. This can save few round trips when reviewer wants space after comma
> etc.

The main problem I see is as soon you start modifing the output from 
spatch by hand you risk to introduce an error and as soon one does 
several rounds of review it one will introduce an error.

For the changes in ConnMan I did applied the semantic patch and then
went over the code base via checkpatch.pl and fixed up the errors.

I don't know if a simple rule like 80 chars max couldn't be added for
the --linux-spacing switch. That was the offender in my case. The
rest of the patch was okay.

cheers,
daniel

[Cocci] Formatting issues

[Julia Lawall]

On Mon, 29 Jul 2013, Daniel Wagner wrote:

> On 07/17/2013 05:51 PM, Ond?ej B?lka wrote:
> > On Wed, Jul 17, 2013 at 05:31:15PM +0200, Julia Lawall wrote:
> > > > Another formatting issue I found is triggered by a different rule. I
> > > > figure
> > > > the expression 'E' is written out as one line and the original
> > > > formatting gets
> > > > lost.
> > >
> > > Yes, there is not much I can do about this one.  When you match code
> > > into a metavariable, all of the whitespace and comments go away.  It
> > > doesn't know that you still want them in the place where you add the
> > > metavariable back in.
> > >
> > > If you have a probleme with this, as in your example, you can try to
> > > rewrite the semantic patch so that the metavariable is there but
> > > does not appear in the - and + code.  For example, for your first
> > > case below you can write
> > >
> > I ran to problems with formating when I tried use cocci in glibc.
> >
> > I think that semantic patch and its formatting are two separate issues.
>
> Thought semantic patching can't just ignore formatting.
>
> > Ideal situation is when project formatting is fully machine specified.
> > Then we can apply patch, run formatter and we are done.
>
> In my case, I can use checkpatch.pl to report some errors.
>
> > Otherwise we can try to convince other developers that benefit of
> > refactoring
> > is bigger than formatting issues it causes or fix it by hand.
> >
> > I now try to convince others to use first option as this allows all
> > submitted patches have perfect formatting because formatter took care of
> > it. This can save few round trips when reviewer wants space after comma
> > etc.
>
> The main problem I see is as soon you start modifing the output from spatch by
> hand you risk to introduce an error and as soon one does several rounds of
> review it one will introduce an error.
>
> For the changes in ConnMan I did applied the semantic patch and then
> went over the code base via checkpatch.pl and fixed up the errors.
>
> I don't know if a simple rule like 80 chars max couldn't be added for
> the --linux-spacing switch. That was the offender in my case. The
> rest of the patch was okay.

It's a simple rule in theory, but not so simple in practice.  Pretty
printing is a hard problem, because doing a good job requires knowing both
what you have seen before and what is coming up.  Also, when Coccinelle is
generating code, it doesn't see the AST any more, only tokens, so it has
limited information.

In any case, the huge indent was just a bug, so hopefully the result will
be somewhat better in the future.

julia

[Cocci] Formatting issues

[Daniel Wagner]

On 07/29/2013 10:49 AM, Julia Lawall wrote:
>>> I now try to convince others to use first option as this allows all
>>> submitted patches have perfect formatting because formatter took care of
>>> it. This can save few round trips when reviewer wants space after comma
>>> etc.
>>
>> The main problem I see is as soon you start modifing the output from spatch by
>> hand you risk to introduce an error and as soon one does several rounds of
>> review it one will introduce an error.
>>
>> For the changes in ConnMan I did applied the semantic patch and then
>> went over the code base via checkpatch.pl and fixed up the errors.
>>
>> I don't know if a simple rule like 80 chars max couldn't be added for
>> the --linux-spacing switch. That was the offender in my case. The
>> rest of the patch was okay.
>
> It's a simple rule in theory, but not so simple in practice.  Pretty
> printing is a hard problem, because doing a good job requires knowing both
> what you have seen before and what is coming up.  Also, when Coccinelle is
> generating code, it doesn't see the AST any more, only tokens, so it has
> limited information.

I understand that in reality this is a hard problem. Something what I 
would helpful here would be a list of lines (emacs/vim parseable) where 
the the result line is too long, presumed that detecting of long lines 
is easy.

> In any case, the huge indent was just a bug, so hopefully the result will
> be somewhat better in the future.

Yes, that one is gone. Thanks again.

Okay, maybe I am overdoing it with my wishlist. I am trying to prepare 
another semantic patch and I see that coccinelle's parser is not too 
happy with certain code. Most of the time I just need to add the right 
macro to my standard.h to make coccinelle happy. My wish would be that 
coccinelle wouldn't just ignore that file instead just stop and yell at 
me: "Parser error: ...". Did I overlook such an option?

cheers,
daniel

[Cocci] Formatting issues

[Julia Lawall]

On Mon, 29 Jul 2013, Daniel Wagner wrote:

> On 07/29/2013 10:49 AM, Julia Lawall wrote:
> > > > I now try to convince others to use first option as this allows all
> > > > submitted patches have perfect formatting because formatter took care of
> > > > it. This can save few round trips when reviewer wants space after comma
> > > > etc.
> > >
> > > The main problem I see is as soon you start modifing the output from
> > > spatch by
> > > hand you risk to introduce an error and as soon one does several rounds of
> > > review it one will introduce an error.
> > >
> > > For the changes in ConnMan I did applied the semantic patch and then
> > > went over the code base via checkpatch.pl and fixed up the errors.
> > >
> > > I don't know if a simple rule like 80 chars max couldn't be added for
> > > the --linux-spacing switch. That was the offender in my case. The
> > > rest of the patch was okay.
> >
> > It's a simple rule in theory, but not so simple in practice.  Pretty
> > printing is a hard problem, because doing a good job requires knowing both
> > what you have seen before and what is coming up.  Also, when Coccinelle is
> > generating code, it doesn't see the AST any more, only tokens, so it has
> > limited information.
>
> I understand that in reality this is a hard problem. Something what I would
> helpful here would be a list of lines (emacs/vim parseable) where the the
> result line is too long, presumed that detecting of long lines is easy.

Perhaps there already exists a tool or emacs mode to do this?

> > In any case, the huge indent was just a bug, so hopefully the result will
> > be somewhat better in the future.
>
> Yes, that one is gone. Thanks again.
>
> Okay, maybe I am overdoing it with my wishlist. I am trying to prepare another
> semantic patch and I see that coccinelle's parser is not too happy with
> certain code. Most of the time I just need to add the right macro to my
> standard.h to make coccinelle happy. My wish would be that coccinelle wouldn't
> just ignore that file instead just stop and yell at me: "Parser error: ...".
> Did I overlook such an option?

It doesn't do this by default, because almost all files contain things it
can't parse, and most of the time the problem is not related to the code
you want to process.  You can use the --verbose-parsing option to get all
of the information about parsing.  But there is a lot of it, and you
should know that Coccinelle makes a number of attempts to parse each
function, and so you can get parse errors on the earlier attempts even
when it succeeds on later attempts.

Usually, it is sufficient to run spatch -parse_c {your code} a few times,
and see what are the top 10 failure tokens that it reports at the end.

julia

[Cocci] Formatting issue

[Daniel Wagner]

On 07/29/2013 11:14 AM, Julia Lawall wrote:
> On Mon, 29 Jul 2013, Daniel Wagner wrote:
>
>> On 07/29/2013 10:49 AM, Julia Lawall wrote:
>>>>> I now try to convince others to use first option as this allows all
>>>>> submitted patches have perfect formatting because formatter took care of
>>>>> it. This can save few round trips when reviewer wants space after comma
>>>>> etc.
>>>>
>>>> The main problem I see is as soon you start modifing the output from
>>>> spatch by
>>>> hand you risk to introduce an error and as soon one does several rounds of
>>>> review it one will introduce an error.
>>>>
>>>> For the changes in ConnMan I did applied the semantic patch and then
>>>> went over the code base via checkpatch.pl and fixed up the errors.
>>>>
>>>> I don't know if a simple rule like 80 chars max couldn't be added for
>>>> the --linux-spacing switch. That was the offender in my case. The
>>>> rest of the patch was okay.
>>>
>>> It's a simple rule in theory, but not so simple in practice.  Pretty
>>> printing is a hard problem, because doing a good job requires knowing both
>>> what you have seen before and what is coming up.  Also, when Coccinelle is
>>> generating code, it doesn't see the AST any more, only tokens, so it has
>>> limited information.
>>
>> I understand that in reality this is a hard problem. Something what I would
>> helpful here would be a list of lines (emacs/vim parseable) where the the
>> result line is too long, presumed that detecting of long lines is easy.
>
> Perhaps there already exists a tool or emacs mode to do this?

Yes, as I said I was falling back to use checkpatch.pl to find the 
offending lines. My point is that coccinelle already knows where it did 
write too long lines, why not just report it?

>>> In any case, the huge indent was just a bug, so hopefully the result will
>>> be somewhat better in the future.
>>
>> Yes, that one is gone. Thanks again.
>>
>> Okay, maybe I am overdoing it with my wishlist. I am trying to prepare another
>> semantic patch and I see that coccinelle's parser is not too happy with
>> certain code. Most of the time I just need to add the right macro to my
>> standard.h to make coccinelle happy. My wish would be that coccinelle wouldn't
>> just ignore that file instead just stop and yell at me: "Parser error: ...".
>> Did I overlook such an option?
>
> It doesn't do this by default, because almost all files contain things it
> can't parse, and most of the time the problem is not related to the code
> you want to process.
 >
> You can use the --verbose-parsing option to get all
> of the information about parsing.  But there is a lot of it, and you
> should know that Coccinelle makes a number of attempts to parse each
> function, and so you can get parse errors on the earlier attempts even
> when it succeeds on later attempts.
 >
> Usually, it is sufficient to run spatch -parse_c {your code} a few times,
> and see what are the top 10 failure tokens that it reports at the end.

Right, '-parse_c' was what I was looking for. Now I see where the parse 
finally chokes.

Hmm, a simple define is too much:

N6ADDR_ALL_DHCP_RELAY_AGENTS_AND_SERVERS_MC_INIT: present in 1 parsing 
errors
example:

        #define IN6ADDR_ALL_DHCP_RELAY_AGENTS_AND_SERVERS_MC_INIT \
         { { { 0xff,0x02,0,0,0,0,0,0,0,0,0,0,0,0x1,0,0x2 } } } /* 
ff02::1:2 */
        static const struct in6_addr 
in6addr_all_dhcp_relay_agents_and_servers_mc =


https://git.kernel.org/cgit/network/connman/connman.git/tree/gdhcp/common.c#n465

cheers,
daniel

[Cocci] Formatting issues

[Ondřej Bílka]

On Mon, Jul 29, 2013 at 11:07:35AM +0200, Daniel Wagner wrote:
> On 07/29/2013 10:49 AM, Julia Lawall wrote:
> >>>I now try to convince others to use first option as this allows all
> >>>submitted patches have perfect formatting because formatter took care of
> >>>it. This can save few round trips when reviewer wants space after comma
> >>>etc.
> >>
> >>The main problem I see is as soon you start modifing the output from spatch by
> >>hand you risk to introduce an error and as soon one does several rounds of
> >>review it one will introduce an error.
> >>
> >>For the changes in ConnMan I did applied the semantic patch and then
> >>went over the code base via checkpatch.pl and fixed up the errors.
> >>
> >>I don't know if a simple rule like 80 chars max couldn't be added for
> >>the --linux-spacing switch. That was the offender in my case. The
> >>rest of the patch was okay.
> >
> >It's a simple rule in theory, but not so simple in practice.  Pretty
> >printing is a hard problem, because doing a good job requires knowing both
> >what you have seen before and what is coming up.  Also, when Coccinelle is
> >generating code, it doesn't see the AST any more, only tokens, so it has
> >limited information.
> 
> I understand that in reality this is a hard problem. Something what
> I would helpful here would be a list of lines (emacs/vim parseable)
> where the the result line is too long, presumed that detecting of
> long lines is easy.
>
I already did it in stylepp. You need to install it
git clone https://github.com/neleai/stylepp/ 
make
# set up $PATH

# Then you can generate list of long lines in files that changed since
# last git commit by
stylepp_warn_long_line --hook
# which generates a list of vim +line file that can be directly ran.
/outputed_path/long_lines


Also splitting lines is one of few problems that needs to be done by
hand, most of other rules have algorithmic description and machine can
do them better than human.

[Cocci] Formatting issues

[Daniel Wagner]

On 07/29/2013 11:28 AM, Ond?ej B?lka wrote:
> On Mon, Jul 29, 2013 at 11:07:35AM +0200, Daniel Wagner wrote:
>> On 07/29/2013 10:49 AM, Julia Lawall wrote:
>>>>> I now try to convince others to use first option as this allows all
>>>>> submitted patches have perfect formatting because formatter took care of
>>>>> it. This can save few round trips when reviewer wants space after comma
>>>>> etc.
>>>>
>>>> The main problem I see is as soon you start modifing the output from spatch by
>>>> hand you risk to introduce an error and as soon one does several rounds of
>>>> review it one will introduce an error.
>>>>
>>>> For the changes in ConnMan I did applied the semantic patch and then
>>>> went over the code base via checkpatch.pl and fixed up the errors.
>>>>
>>>> I don't know if a simple rule like 80 chars max couldn't be added for
>>>> the --linux-spacing switch. That was the offender in my case. The
>>>> rest of the patch was okay.
>>>
>>> It's a simple rule in theory, but not so simple in practice.  Pretty
>>> printing is a hard problem, because doing a good job requires knowing both
>>> what you have seen before and what is coming up.  Also, when Coccinelle is
>>> generating code, it doesn't see the AST any more, only tokens, so it has
>>> limited information.
>>
>> I understand that in reality this is a hard problem. Something what
>> I would helpful here would be a list of lines (emacs/vim parseable)
>> where the the result line is too long, presumed that detecting of
>> long lines is easy.
>>
> I already did it in stylepp. You need to install it

Thanks, I give it a try.

cheers,
daniel

[Cocci] Formatting issues

[Ondřej Bílka]

On Mon, Jul 29, 2013 at 04:59:30PM +0200, Daniel Wagner wrote:
> On 07/29/2013 11:28 AM, Ond?ej B?lka wrote:
> >On Mon, Jul 29, 2013 at 11:07:35AM +0200, Daniel Wagner wrote:
> >>On 07/29/2013 10:49 AM, Julia Lawall wrote:
> >>>It's a simple rule in theory, but not so simple in practice.  Pretty
> >>>printing is a hard problem, because doing a good job requires knowing both
> >>>what you have seen before and what is coming up.  Also, when Coccinelle is
> >>>generating code, it doesn't see the AST any more, only tokens, so it has
> >>>limited information.
> >>
> >>I understand that in reality this is a hard problem. Something what
> >>I would helpful here would be a list of lines (emacs/vim parseable)
> >>where the the result line is too long, presumed that detecting of
> >>long lines is easy.
> >>
> >I already did it in stylepp. You need to install it
> 
> Thanks, I give it a try.
> 
I now also wrote a similar tool,

stylepp_restrict_formater

It reverts changes on lines that were not touched in last commit so you
could use following:

apply patch,
git commit
for I in `git diff --name-only HEAD^`; do
  run favourite formatter
done
stylepp_restrict_formatter

It does not handle formatters that split lines yet.
A source is at script/stylepp_restrict_formatter src/_restrict_formatter.c 
so you could try to add it.

[Cocci] Formatting issues

[Daniel Wagner]

On 07/29/2013 05:59 PM, Ond?ej B?lka wrote:
> On Mon, Jul 29, 2013 at 04:59:30PM +0200, Daniel Wagner wrote:
>> On 07/29/2013 11:28 AM, Ond?ej B?lka wrote:
>>> On Mon, Jul 29, 2013 at 11:07:35AM +0200, Daniel Wagner wrote:
>>>> On 07/29/2013 10:49 AM, Julia Lawall wrote:
>>>>> It's a simple rule in theory, but not so simple in practice.  Pretty
>>>>> printing is a hard problem, because doing a good job requires knowing both
>>>>> what you have seen before and what is coming up.  Also, when Coccinelle is
>>>>> generating code, it doesn't see the AST any more, only tokens, so it has
>>>>> limited information.
>>>>
>>>> I understand that in reality this is a hard problem. Something what
>>>> I would helpful here would be a list of lines (emacs/vim parseable)
>>>> where the the result line is too long, presumed that detecting of
>>>> long lines is easy.
>>>>
>>> I already did it in stylepp. You need to install it
>>
>> Thanks, I give it a try.
>>
> I now also wrote a similar tool,
>
> stylepp_restrict_formater
>
> It reverts changes on lines that were not touched in last commit so you
> could use following:
>
> apply patch,
> git commit
> for I in `git diff --name-only HEAD^`; do
>    run favourite formatter
> done
> stylepp_restrict_formatter

Thanks for the tipp. Since we are using pretty much the kernel codying 
style I just used checkpath.pl, that is

git diff | checkpatch.pl -

as compile command in emacs. Since emacs does not understand the 
error/warnings from checkpatch.pl (--emacs doesn't help because
the input file name is missing), I added a new compilation error regex 
rule to emacs. With that I can easly jump to the offendling lines and 
edit directly from emacs.

cheers,
daniel

[Cocci] Formatting issues

[Julia Lawall]

On Wed, 17 Jul 2013, Daniel Wagner wrote:

> Hi,
> 
> I have found another small issue. Not a big thing. I have following rule
> 
> @@
> identifier f =~ "^(__)?connman_.*" ;
> @@
> 
> f(...,
> (
> - FALSE
> + false
> |
> - TRUE
> + true
> )
>  ,...)
> 
> 
> And this little C example:
> 
> bool __connman_bar(bool baz);
> int connman_foo(int val, bool bar);
> 
> int main(int argc, char *argv)
> {
> 	int err;
> 
> 	if (__connman_bar(FALSE) == TRUE)
> 		err = connman_a_rather_long_line(2434, TRUE);
> 
> 	return 0;
> }
> 
> 
> Running coccinelle on this results in the not so nicely formated patch:
> 
> @@ -5,8 +5,9 @@ int main(int argc, char *argv)
>  {
>         int err;
>  
> -       if (__connman_bar(FALSE) == TRUE)
> -               err = connman_a_rather_long_line(2434, TRUE);
> +       if (__connman_bar(false) == TRUE)
> +               err = connman_a_rather_long_line(2434,
> +                                                                         true);
>  
>         return 0;
>  }
> 
> When connman_a_rather_long_line() is not sooo long then it works as
> expected, that means not additional line is introduced. Any ideas
> what is going wrong?

A patch is below.

julia

diff --git a/parsing_c/unparse_c.ml b/parsing_c/unparse_c.ml
index 971024a..9e03343 100644
--- a/parsing_c/unparse_c.ml
+++ b/parsing_c/unparse_c.ml
@@ -1071,7 +1071,7 @@ let add_newlines toks tabbing_unit =
             let (newcount,newstack,newspacecell) =
               start_box stack space_cell newcount "{" in
             front @ loop (newstack,newspacecell) newcount ixs
-          | s -> a :: loop info (string_length s count) xs
+          | _ -> a :: loop info (string_length s count) xs
           )
         | _ -> a :: loop info (string_length s count) xs
         )
@@ -1114,8 +1114,7 @@ let add_newlines toks tabbing_unit =
           | [x] ->
             (match check_for_newline count x space_cell with
             | Some count -> loop (stack,Some (x,sp)) count xs
-            | None -> loop (stack,Some (count,sp)) count xs
-	    )
+            | None -> loop (stack,Some (count,sp)) count xs)
           | _ -> loop info count xs
           ) in
       a :: rest

[Cocci] Formatting issues

[Daniel Wagner]

Hi Julia,

On 07/28/2013 12:48 PM, Julia Lawall wrote:
> On Wed, 17 Jul 2013, Daniel Wagner wrote:
>
>> Hi,
>>
>> I have found another small issue. Not a big thing. I have following rule
>>
>> @@
>> identifier f =~ "^(__)?connman_.*" ;
>> @@
>>
>> f(...,
>> (
>> - FALSE
>> + false
>> |
>> - TRUE
>> + true
>> )
>>   ,...)
>>
>>
>> And this little C example:
>>
>> bool __connman_bar(bool baz);
>> int connman_foo(int val, bool bar);
>>
>> int main(int argc, char *argv)
>> {
>> 	int err;
>>
>> 	if (__connman_bar(FALSE) == TRUE)
>> 		err = connman_a_rather_long_line(2434, TRUE);
>>
>> 	return 0;
>> }
>>
>>
>> Running coccinelle on this results in the not so nicely formated patch:
>>
>> @@ -5,8 +5,9 @@ int main(int argc, char *argv)
>>   {
>>          int err;
>>
>> -       if (__connman_bar(FALSE) == TRUE)
>> -               err = connman_a_rather_long_line(2434, TRUE);
>> +       if (__connman_bar(false) == TRUE)
>> +               err = connman_a_rather_long_line(2434,
>> +                                                                         true);
>>
>>          return 0;
>>   }
>>
>> When connman_a_rather_long_line() is not sooo long then it works as
>> expected, that means not additional line is introduced. Any ideas
>> what is going wrong?
>
> A patch is below.

Patch works. Thanks.

cheers,
daniel