[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Kieran Bingham]

Reviewing the changes made by my s-patch, I've come across the following hunk :

============================================================
--- a/drivers/hwmon/jc42.c
+++ b/drivers/hwmon/jc42.c
...
@@ -529,13 +529,7 @@ static const struct dev_pm_ops jc42_dev_pm_ops = {
 #define JC42_DEV_PM_OPS (&jc42_dev_pm_ops)
 #else
 #define JC42_DEV_PM_OPS NULL
-#endif /* CONFIG_PM */
-
-static const struct i2c_device_id jc42_id[] = {
-       { "jc42", 0 },
-       { }
-};
-MODULE_DEVICE_TABLE(i2c, jc42_id);
+#endif

 static struct i2c_driver jc42_driver = {
        .class          = I2C_CLASS_SPD,
============================================================

Is this removal of the /* CONFIG_PM */ expected behaviour?

I guess under the hood, Coccinelle is stripping comments from code to
make parsing possible/easier?

--
Regards

Kieran

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[SF Markus Elfring]

> Reviewing the changes made by my s-patch, I've come across the following hunk :

How do you think about to show your semantic patch script here?


> +++ b/drivers/hwmon/jc42.c
> ...
> @@ -529,13 +529,7 @@ static const struct dev_pm_ops jc42_dev_pm_ops = {
>  #define JC42_DEV_PM_OPS (&jc42_dev_pm_ops)
>  #else
>  #define JC42_DEV_PM_OPS NULL
> -#endif /* CONFIG_PM */
> -
> -static const struct i2c_device_id jc42_id[] = {
> -       { "jc42", 0 },
> -       { }
> -};
> -MODULE_DEVICE_TABLE(i2c, jc42_id);
> +#endif
>
>  static struct i2c_driver jc42_driver = {
>         .class          = I2C_CLASS_SPD,
> ============================================================
>
> Is this removal of the /* CONFIG_PM */ expected behaviour?

I would interpret the shown changes in the way that a preprocessor
statement "#endif" is repositioned after a variable initialisation which
was not a part for conditional compilation is deleted. I find such a
detail also strange.


> I guess under the hood, Coccinelle is stripping comments from code to
> make parsing possible/easier?

This software is tackling some challenges to preserve comments for
various source code places.

Regards,
Markus

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Kieran Bingham]

Hi Markus,

On 9 September 2015 at 13:14, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> Reviewing the changes made by my s-patch, I've come across the following hunk :
>
> How do you think about to show your semantic patch script here?

My apologies, I should have considered that!
This is actually my second thread to the list today - the first is
still held in moderation I think. (posted before I joined the list)

The full patch was referenced in the first thread, so in my head was
just continuing as if there was context which I appreciate there isn't
:)

Full patch on viewable here:
https://gist.github.com/kbingham/96477177dd20a72b1c2f


The relevant segment of the patch is:
============================================================
// Remove the i2c_device_id array

@ dev_id depends on of_dev_id_present @
identifier arr;
@@
- struct i2c_device_id arr[] = { ... };


// Now remove the MODULE_DEVICE_TABLE entry
@ depends on dev_id @
declarer name MODULE_DEVICE_TABLE;
identifier i2c;
identifier dev_id.arr;
@@
- MODULE_DEVICE_TABLE(i2c, arr);
============================================================


>> +++ b/drivers/hwmon/jc42.c
>> ...
>> @@ -529,13 +529,7 @@ static const struct dev_pm_ops jc42_dev_pm_ops = {
>>  #define JC42_DEV_PM_OPS (&jc42_dev_pm_ops)
>>  #else
>>  #define JC42_DEV_PM_OPS NULL
>> -#endif /* CONFIG_PM */
>> -
>> -static const struct i2c_device_id jc42_id[] = {
>> -       { "jc42", 0 },
>> -       { }
>> -};
>> -MODULE_DEVICE_TABLE(i2c, jc42_id);
>> +#endif
>>
>>  static struct i2c_driver jc42_driver = {
>>         .class          = I2C_CLASS_SPD,
>> ============================================================
>>
>> Is this removal of the /* CONFIG_PM */ expected behaviour?
>
> I would interpret the shown changes in the way that a preprocessor
> statement "#endif" is repositioned after a variable initialisation which
> was not a part for conditional compilation is deleted. I find such a
> detail also strange.
>
>
>> I guess under the hood, Coccinelle is stripping comments from code to
>> make parsing possible/easier?
>
> This software is tackling some challenges to preserve comments for
> various source code places.

Having only got started with Coccinelle yesterday, I find it
fascinating how well it performs.
Its a marvellous tool, and I wish I had taken the energy to start
using it long before now!

> Regards,
> Markus

Regards

Kieran

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Julia Lawall]

On Wed, 9 Sep 2015, Kieran Bingham wrote:

> Reviewing the changes made by my s-patch, I've come across the following hunk :
>
> ============================================================
> --- a/drivers/hwmon/jc42.c
> +++ b/drivers/hwmon/jc42.c
> ...
> @@ -529,13 +529,7 @@ static const struct dev_pm_ops jc42_dev_pm_ops = {
>  #define JC42_DEV_PM_OPS (&jc42_dev_pm_ops)
>  #else
>  #define JC42_DEV_PM_OPS NULL
> -#endif /* CONFIG_PM */
> -
> -static const struct i2c_device_id jc42_id[] = {
> -       { "jc42", 0 },
> -       { }
> -};
> -MODULE_DEVICE_TABLE(i2c, jc42_id);
> +#endif
>
>  static struct i2c_driver jc42_driver = {
>         .class          = I2C_CLASS_SPD,
> ============================================================
>
> Is this removal of the /* CONFIG_PM */ expected behaviour?
>
> I guess under the hood, Coccinelle is stripping comments from code to
> make parsing possible/easier?

Not at all. It sees this as a comment before the start of a top level
declaration, and considers that since you don't want the declaration any
more, you don't want the comment either.

It would need to observe that the comment does not start at the beginning
of the line, and treat it differntly.  Just to check, are you using
Coccinelle 1.0.2?  I made a change in this regard recently, but I'm not
sure it was an overall improvement.

julia

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Kieran Bingham]

Hi Julia

On 9 September 2015 at 15:01, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Wed, 9 Sep 2015, Kieran Bingham wrote:
>
>> Reviewing the changes made by my s-patch, I've come across the following hunk :
>>
>> ============================================================
>> --- a/drivers/hwmon/jc42.c
>> +++ b/drivers/hwmon/jc42.c
>> ...
>> @@ -529,13 +529,7 @@ static const struct dev_pm_ops jc42_dev_pm_ops = {
>>  #define JC42_DEV_PM_OPS (&jc42_dev_pm_ops)
>>  #else
>>  #define JC42_DEV_PM_OPS NULL
>> -#endif /* CONFIG_PM */
>> -
>> -static const struct i2c_device_id jc42_id[] = {
>> -       { "jc42", 0 },
>> -       { }
>> -};
>> -MODULE_DEVICE_TABLE(i2c, jc42_id);
>> +#endif
>>
>>  static struct i2c_driver jc42_driver = {
>>         .class          = I2C_CLASS_SPD,
>> ============================================================
>>
>> Is this removal of the /* CONFIG_PM */ expected behaviour?
>>
>> I guess under the hood, Coccinelle is stripping comments from code to
>> make parsing possible/easier?
>
> Not at all. It sees this as a comment before the start of a top level
> declaration, and considers that since you don't want the declaration any
> more, you don't want the comment either.
>


Ahh yes, I see this here! (Actually sounds like a reasonable thing to do!)
@@ -207,37 +207,6 @@ enum chips { lm90, adm1032, lm99, lm86, max6657,
max6659, adt7461, max6680,
 #define MAX6696_STATUS2_LOT2   (1 << 7) /* local emergency limit tripped */

 /*
- * Driver data (common to all clients)
- */
-
-static const struct i2c_device_id lm90_id[] = {
-       { "adm1032", adm1032 },
-       { "adt7461", adt7461 },
-       { "adt7461a", adt7461 },
-       { "g781", g781 },






> It would need to observe that the comment does not start at the beginning
> of the line, and treat it differntly.  Just to check, are you using
> Coccinelle 1.0.2?  I made a change in this regard recently, but I'm not
> sure it was an overall improvement.

No - I'm afraid I'm using the version shipped with ubuntu 15.04:

spatch --version
spatch version 1.0.0-rc22 with Python support and with PCRE support


--
Regards

Kieran

>
> julia

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Julia Lawall]

On Wed, 9 Sep 2015, Kieran Bingham wrote:

> Hi Julia
>
> On 9 September 2015 at 15:01, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> > On Wed, 9 Sep 2015, Kieran Bingham wrote:
> >
> >> Reviewing the changes made by my s-patch, I've come across the following hunk :
> >>
> >> ============================================================
> >> --- a/drivers/hwmon/jc42.c
> >> +++ b/drivers/hwmon/jc42.c
> >> ...
> >> @@ -529,13 +529,7 @@ static const struct dev_pm_ops jc42_dev_pm_ops = {
> >>  #define JC42_DEV_PM_OPS (&jc42_dev_pm_ops)
> >>  #else
> >>  #define JC42_DEV_PM_OPS NULL
> >> -#endif /* CONFIG_PM */
> >> -
> >> -static const struct i2c_device_id jc42_id[] = {
> >> -       { "jc42", 0 },
> >> -       { }
> >> -};
> >> -MODULE_DEVICE_TABLE(i2c, jc42_id);
> >> +#endif
> >>
> >>  static struct i2c_driver jc42_driver = {
> >>         .class          = I2C_CLASS_SPD,
> >> ============================================================
> >>
> >> Is this removal of the /* CONFIG_PM */ expected behaviour?
> >>
> >> I guess under the hood, Coccinelle is stripping comments from code to
> >> make parsing possible/easier?
> >
> > Not at all. It sees this as a comment before the start of a top level
> > declaration, and considers that since you don't want the declaration any
> > more, you don't want the comment either.
> >
>
>
> Ahh yes, I see this here! (Actually sounds like a reasonable thing to do!)
> @@ -207,37 +207,6 @@ enum chips { lm90, adm1032, lm99, lm86, max6657,
> max6659, adt7461, max6680,
>  #define MAX6696_STATUS2_LOT2   (1 << 7) /* local emergency limit tripped */
>
>  /*

Is there really no - here?

julia

> - * Driver data (common to all clients)
> - */
> -
> -static const struct i2c_device_id lm90_id[] = {
> -       { "adm1032", adm1032 },
> -       { "adt7461", adt7461 },
> -       { "adt7461a", adt7461 },
> -       { "g781", g781 },
>
>
>
>
>
>
> > It would need to observe that the comment does not start at the beginning
> > of the line, and treat it differntly.  Just to check, are you using
> > Coccinelle 1.0.2?  I made a change in this regard recently, but I'm not
> > sure it was an overall improvement.
>
> No - I'm afraid I'm using the version shipped with ubuntu 15.04:
>
> spatch --version
> spatch version 1.0.0-rc22 with Python support and with PCRE support
>
>
> --
> Regards
>
> Kieran
>
> >
> > julia
>

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Kieran Bingham]

Hi Julia,


>>
>> Ahh yes, I see this here! (Actually sounds like a reasonable thing to do!)
>> @@ -207,37 +207,6 @@ enum chips { lm90, adm1032, lm99, lm86, max6657,
>> max6659, adt7461, max6680,
>>  #define MAX6696_STATUS2_LOT2   (1 << 7) /* local emergency limit tripped */
>>
>>  /*
>
> Is there really no - here?

Now that's a keen eye you have!

Yes - that is correct, but fear not ... its my fault for cropping the hunk.

There is a comment section below, and the hunk removes the first /*
there instead.


Full hunk below: I'll try not to 'cut corners' when pasting next time :)


diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index c9ff08d..5b76055 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -207,37 +207,6 @@ enum chips { lm90, adm1032, lm99, lm86, max6657,
max6659, adt7461, max6680,
 #define MAX6696_STATUS2_LOT2   (1 << 7) /* local emergency limit tripped */

 /*
- * Driver data (common to all clients)
- */
-
-static const struct i2c_device_id lm90_id[] = {
-       { "adm1032", adm1032 },
-       { "adt7461", adt7461 },
-       { "adt7461a", adt7461 },
-       { "g781", g781 },
-       { "lm90", lm90 },
-       { "lm86", lm86 },
-       { "lm89", lm86 },
-       { "lm99", lm99 },
-       { "max6646", max6646 },
-       { "max6647", max6646 },
-       { "max6649", max6646 },
-       { "max6657", max6657 },
-       { "max6658", max6657 },
-       { "max6659", max6659 },
-       { "max6680", max6680 },
-       { "max6681", max6680 },
-       { "max6695", max6696 },
-       { "max6696", max6696 },
-       { "nct1008", adt7461 },
-       { "w83l771", w83l771 },
-       { "sa56004", sa56004 },
-       { "tmp451", tmp451 },
-       { }
-};
-MODULE_DEVICE_TABLE(i2c, lm90_id);
-
-/*
  * chip type specific parameters
  */
 struct lm90_params {



--
Regards

Kieran


>
> julia
>
>> - * Driver data (common to all clients)
>> - */
>> -
>> -static const struct i2c_device_id lm90_id[] = {
>> -       { "adm1032", adm1032 },
>> -       { "adt7461", adt7461 },
>> -       { "adt7461a", adt7461 },
>> -       { "g781", g781 },
>>
>>
>>
>>
>>
>>
>> > It would need to observe that the comment does not start at the beginning
>> > of the line, and treat it differntly.  Just to check, are you using
>> > Coccinelle 1.0.2?  I made a change in this regard recently, but I'm not
>> > sure it was an overall improvement.
>>
>> No - I'm afraid I'm using the version shipped with ubuntu 15.04:
>>
>> spatch --version
>> spatch version 1.0.0-rc22 with Python support and with PCRE support
>>
>>
>> --
>> Regards
>>
>> Kieran
>>
>> >
>> > julia
>>

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[SF Markus Elfring]

> There is a comment section below, and the hunk removes the first /*
> there instead.

Does this generated update suggestion fit to your expectations?

Would you like to express any preferences if specific comments should be
deleted together with some source code or when such annotations should
be preserved for a preprocessor statement?

Regards,
Markus

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Kieran Bingham]

On 9 September 2015 at 15:48, SF Markus Elfring
<elfring@users.sourceforge.net> wrote:
>> There is a comment section below, and the hunk removes the first /*
>> there instead.
>
> Does this generated update suggestion fit to your expectations?
>
> Would you like to express any preferences if specific comments should be
> deleted together with some source code or when such annotations should
> be preserved for a preprocessor statement?

I was surprised to see it happen, as I was not expecting it, however I think it
is a useful feature.

In the most part, the code blocks I am removing, if they are titled
with a comment,
would also want that comment to be removed.

(with the caveat that the comment is not attached to the end of
another statement as discovered)


In this particular instance, the fact that the alignment of the
comment being removed from below, is just something that happens in
patches from diff, and I think for me that is expected behaviour,
(Though if anyone knows how to stop it happening I'd be interested in
hearing) but I believe it is not an issue related to Coccinelle.

Regards

Kieran


>
> Regards,
> Markus

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Julia Lawall]

The following patch should hopefully remove the problem of undesrably
removing the comment.  Let me know if there are further such problems.

thanks,
julia

---

diff --git a/parsing_c/unparse_c.ml b/parsing_c/unparse_c.ml
index 7240165..3879df5 100644
--- a/parsing_c/unparse_c.ml
+++ b/parsing_c/unparse_c.ml
@@ -272,9 +272,9 @@ let get_fakeInfo_and_tokens celem toks =

 (* Fake nodes that have BEFORE code or are - should be moved over any
 subsequent whitespace and newlines, but not any comments starting in column
-0, to get as close to the affected code as possible.  Similarly, fake nodes
-that have AFTER code should be moved backwards.  No fake nodes should have
-both before and after code. *)
+0 or the last newline, to get as close to the affected code as possible.
+Similarly, fake nodes that have AFTER code should be moved backwards.  No
+fake nodes should have both before and after code. *)

 let displace_fake_nodes toks =
   let is_fake = function Fake1 _ -> true | _ -> false in
@@ -286,6 +286,13 @@ let displace_fake_nodes toks =
 	(* column 0 is the leftmost column. *)
 	Ast_c.col_of_info i > 0
     | _ -> false in
+  let is_nonnl_whitespace_or_noncol0_comment = function
+    | T1(Parser_c.TCommentSpace _) -> true
+  (* patch: cocci    *)
+    | T1(Parser_c.TComment i) ->
+	(* column 0 is the leftmost column. *)
+	Ast_c.col_of_info i > 0
+    | _ -> false in
   let is_whitespace_or_comment = function
     | T1(Parser_c.TCommentSpace _)
     | T1(Parser_c.TCommentNewline _)
@@ -316,6 +323,11 @@ let displace_fake_nodes toks =
           (* move the fake node forwards *)
           let (whitespace,rest) = span is_whitespace_or_noncol0_comment aft in
           bef @ whitespace @ fake :: (loop rest)
+        | (Ast_cocci.MINUS(_,_,_,Ast_cocci.NOREPLACEMENT),_) ->
+          (* move the fake node forwards *)
+          let (whitespace,rest) =
+	    span is_nonnl_whitespace_or_noncol0_comment aft in
+          bef @ whitespace @ fake :: (loop rest)
         | (Ast_cocci.CONTEXT(_,Ast_cocci.AFTER _),_) ->
           (* move the fake node backwards *)
           let revbef = List.rev bef in

[Cocci] Comments removed on (adjacent) lines not touched ... ?

[Julia Lawall]

> > It would need to observe that the comment does not start at the beginning
> > of the line, and treat it differntly.  Just to check, are you using
> > Coccinelle 1.0.2?  I made a change in this regard recently, but I'm not
> > sure it was an overall improvement.
>
> No - I'm afraid I'm using the version shipped with ubuntu 15.04:
>
> spatch --version
> spatch version 1.0.0-rc22 with Python support and with PCRE support

1.0.2 is still broken in this respect.  I will try to fix it.

julia