[Cocci] broken comments handling

[Cocci] broken comments handling

[Cyril Hrubis]

Hi!
I've found potentialy harmful bug while using spatch, see below.

bug.c:
int a; /* This is comment for a */
int b; /* This is comment for b */

bug.cocci:
@@ @@
-int b;

spatch-23 bug.cocci bug.c
init_defs_builtins: /usr/local/share/coccinelle/standard.h
warning: line 2: should b be a metavariable?
HANDLING: bug.c
diff =
--- bug.c
+++ /tmp/cocci-output-2227-08362d-bug.c
@@ -1,2 +1 @@
-int a; /* This is comment for a */
-int b; /* This is comment for b */
+int a; /* This is comment for b */

The comment for a is removed and replaced with the comment for b, which may
lead to serious confusion.

-- 
Cyril Hrubis
chrubis at suse.cz

[Cocci] broken comments handling

[Julia Lawall]

On Wed, 4 Feb 2015, Cyril Hrubis wrote:

> Hi!
> I've found potentialy harmful bug while using spatch, see below.
>
> bug.c:
> int a; /* This is comment for a */
> int b; /* This is comment for b */
>
> bug.cocci:
> @@ @@
> -int b;
>
> spatch-23 bug.cocci bug.c
> init_defs_builtins: /usr/local/share/coccinelle/standard.h
> warning: line 2: should b be a metavariable?
> HANDLING: bug.c
> diff =
> --- bug.c
> +++ /tmp/cocci-output-2227-08362d-bug.c
> @@ -1,2 +1 @@
> -int a; /* This is comment for a */
> -int b; /* This is comment for b */
> +int a; /* This is comment for b */
>
> The comment for a is removed and replaced with the comment for b, which may
> lead to serious confusion.

Thanks.

I would have expected that both comments would stay.  I will look into it.
Thanks for the report.

julia

[Cocci] broken comments handling

[Cyril Hrubis]

Hi!
> > The comment for a is removed and replaced with the comment for b, which may
> > lead to serious confusion.
> 
> Thanks.
> 
> I would have expected that both comments would stay.  I will look into it.
> Thanks for the report.

In this case removing the comment for the removed variable makes more
sense to me.

-- 
Cyril Hrubis
chrubis at suse.cz

[Cocci] broken comments handling

[Julia Lawall]

On Wed, 4 Feb 2015, Cyril Hrubis wrote:

> Hi!
> > > The comment for a is removed and replaced with the comment for b, which may
> > > lead to serious confusion.
> >
> > Thanks.
> >
> > I would have expected that both comments would stay.  I will look into it.
> > Thanks for the report.
>
> In this case removing the comment for the removed variable makes more
> sense to me.

Sure but Coccinelle is not always that clever...

julia

[Cocci] Source code adjustments together with comments?

[SF Markus Elfring]

>> I would have expected that both comments would stay.  I will look into it.
>> Thanks for the report.
> 
> In this case removing the comment for the removed variable makes more
> sense to me.

Would you like to be more explicit in the semantic patch language
around source code adjustments which should also affect
corresponding comments?

Regards,
Markus

[Cocci] Source code adjustments together with comments?

[Julia Lawall]

On Wed, 4 Feb 2015, SF Markus Elfring wrote:

> >> I would have expected that both comments would stay.  I will look into it.
> >> Thanks for the report.
> >
> > In this case removing the comment for the removed variable makes more
> > sense to me.
>
> Would you like to be more explicit in the semantic patch language
> around source code adjustments which should also affect
> corresponding comments?

It seems complicated, and people don't always follow the same comventions.

Probably it is thinking that the code looks like this:

/*some comment on b */
int b;

I could change it so that this strategy is only followed if /*some comment
on b */ starts at the beginning of the line.

julia

[Cocci] Source code adjustments together with comments?

[Cyril Hrubis]

Hi!
> > In this case removing the comment for the removed variable makes more
> > sense to me.
> 
> Would you like to be more explicit in the semantic patch language
> around source code adjustments which should also affect
> corresponding comments?

I would rather see it work right (tm). The thing is that I do not use
the tool that often. It comes very handy in large scale fixes and
cleanups but the more complicated the patch language is the the higher
is the bar for actual usage.

I wouldn't mind having more options but I doubt I would need these. And
the tool should, in my opinion, do what is the most reasonable action by
default.

-- 
Cyril Hrubis
chrubis at suse.cz

[Cocci] Source code adjustments together with comments?

[Cyril Hrubis]

Hi!
> > Would you like to be more explicit in the semantic patch language
> > around source code adjustments which should also affect
> > corresponding comments?
> 
> It seems complicated, and people don't always follow the same comventions.
> 
> Probably it is thinking that the code looks like this:
> 
> /*some comment on b */
> int b;

That explains it.

> I could change it so that this strategy is only followed if /*some comment
> on b */ starts at the beginning of the line.

That sounds good. What would be the strategy for the other case then?

-- 
Cyril Hrubis
chrubis at suse.cz

[Cocci] Source code adjustments together with comments?

[Julia Lawall]

> > I could change it so that this strategy is only followed if /*some comment
> > on b */ starts at the beginning of the line.
>
> That sounds good. What would be the strategy for the other case then?

I expect that you will end up with both comments.  Perhaps it could be
possible to detect that the remaining line only contains a comment and
delete that.  I guess that would be better.

julia

[Cocci] Source code adjustments together with comments?

[SF Markus Elfring]

> And the tool should, in my opinion, do what is the most reasonable
> action by default.

I guess that there are some software development challenges for
further considerations.

Which rules do you use for the connection of a specific amount
of comments to statements in other source code places?

Would you like to express any preferences for consistent
software builds?

Regards,
Markus

[Cocci] Source code adjustments together with comments?

[Rasmus Villemoes]

On Wed, Feb 04 2015, Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
>> > Would you like to be more explicit in the semantic patch language
>> > around source code adjustments which should also affect
>> > corresponding comments?
>> 
>> It seems complicated, and people don't always follow the same comventions.
>> 
>> Probably it is thinking that the code looks like this:
>> 
>> /*some comment on b */
>> int b;
>
> That explains it.
>
>> I could change it so that this strategy is only followed if /*some comment
>> on b */ starts at the beginning of the line.
>
> That sounds good. What would be the strategy for the other case then?

It seems to be a reasonable heuristic that a comment beginning on a line
which also contains code is attached to that code, while a comment
beginning on a line by itself is attached to the following code (whether
that means one or more lines of code is of course impossible to guess).

So, in the former case, if the code is simply removed, the comment should
also vanish. But what if the semantic patch contains + code? Something
like

- int hash;
+ unsigned int hash;

In that case, it's probably best to leave the comment. But one can
probably always find examples where whatever coccinelle does, something
else would have been better. For example, if the comment should stay but
needs rewording, good luck teaching any computer program to do that.

In short, nothing can save one from doing a manual review of the
generated patch, especially when comments are involved.

Rasmus

[Cocci] Source code adjustments together with comments?

[Julia Lawall]

On Wed, 4 Feb 2015, Rasmus Villemoes wrote:

> On Wed, Feb 04 2015, Cyril Hrubis <chrubis@suse.cz> wrote:
>
> > Hi!
> >> > Would you like to be more explicit in the semantic patch language
> >> > around source code adjustments which should also affect
> >> > corresponding comments?
> >>
> >> It seems complicated, and people don't always follow the same comventions.
> >>
> >> Probably it is thinking that the code looks like this:
> >>
> >> /*some comment on b */
> >> int b;
> >
> > That explains it.
> >
> >> I could change it so that this strategy is only followed if /*some comment
> >> on b */ starts at the beginning of the line.
> >
> > That sounds good. What would be the strategy for the other case then?
>
> It seems to be a reasonable heuristic that a comment beginning on a line
> which also contains code is attached to that code, while a comment
> beginning on a line by itself is attached to the following code (whether
> that means one or more lines of code is of course impossible to guess).
>
> So, in the former case, if the code is simply removed, the comment should
> also vanish. But what if the semantic patch contains + code? Something
> like
>
> - int hash;
> + unsigned int hash;
>
> In that case, it's probably best to leave the comment. But one can
> probably always find examples where whatever coccinelle does, something
> else would have been better. For example, if the comment should stay but
> needs rewording, good luck teaching any computer program to do that.
>
> In short, nothing can save one from doing a manual review of the
> generated patch, especially when comments are involved.

I agree.  But I also agree with Cyril that the current situation is not
desirable, because the result is quite confusing.

julia

[Cocci] Source code adjustments together with comments?

[Cyril Hrubis]

Hi!
> > And the tool should, in my opinion, do what is the most reasonable
> > action by default.
> 
> I guess that there are some software development challenges for
> further considerations.

Indeed, unfortunatelly I do not expect this to be solved without some
trial and error.

> Which rules do you use for the connection of a specific amount
> of comments to statements in other source code places?

I may see this overly simplistic but there are two types of comment in
common use. Comments on a separate line(s) that most of the time belongs
to the closest code block. Then there are comments that continue after
the semicolon to the end of the line and these to belong to the same
line as the code beforehand. There may be far more strange special
cases, but for 99% of situations these two classes should work. But as I
said I do not expect this to be solved without some trial and error.

-- 
Cyril Hrubis
chrubis at suse.cz