[PATCH] Staging: iio: adc: Added Space around binary op.

[PATCH] Staging: iio: adc: Added Space around binary op.

[Himanshi Jain]

Added space around(one on each side of) binary
operator(-) as preferred according to kernel
coding style.

Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
---
 drivers/staging/iio/adc/ad7192.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
index d11c6de..1aee662 100644
--- a/drivers/staging/iio/adc/ad7192.c
+++ b/drivers/staging/iio/adc/ad7192.c
@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
 }
 
 static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
-			     in_voltage-voltage_scale_available,
+			     in_voltage - voltage_scale_available,
 			     0444, ad7192_show_scale_available, NULL, 0);
 
 static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
-- 
1.9.1

Re: [Outreachy kernel] [PATCH] Staging: iio: adc: Added Space around binary op.

[Julia Lawall]

On Fri, 8 Sep 2017, Himanshi Jain wrote:

> Added space around(one on each side of) binary

I think that just around would be clear enough.

In the previous patches on this file, found with git log --oneline, a
subject line of staging: iio: ad7192: seems to be more popular when the
patch affects only this file.  But maybe it doesn't matter either way.

julia

> operator(-) as preferred according to kernel
> coding style.
>
> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> ---
>  drivers/staging/iio/adc/ad7192.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/adc/ad7192.c b/drivers/staging/iio/adc/ad7192.c
> index d11c6de..1aee662 100644
> --- a/drivers/staging/iio/adc/ad7192.c
> +++ b/drivers/staging/iio/adc/ad7192.c
> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
>  }
>
>  static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> -			     in_voltage-voltage_scale_available,
> +			     in_voltage - voltage_scale_available,
>  			     0444, ad7192_show_scale_available, NULL, 0);
>
>  static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
> --
> 1.9.1
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20170908044752.GA6199%40himanshi-Inspiron-5558.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Jonathan Cameron]

On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
>Added space around(one on each side of) binary
>operator(-) as preferred according to kernel
>coding style.
>
>Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>

Take a closer look at that macro. It isn't doing what you think...  To give a hint, changing this breaks userspace.

Jonathan


>---
> drivers/staging/iio/adc/ad7192.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/staging/iio/adc/ad7192.c
>b/drivers/staging/iio/adc/ad7192.c
>index d11c6de..1aee662 100644
>--- a/drivers/staging/iio/adc/ad7192.c
>+++ b/drivers/staging/iio/adc/ad7192.c
>@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> }
> 
> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
>-			     in_voltage-voltage_scale_available,
>+			     in_voltage - voltage_scale_available,
> 			     0444, ad7192_show_scale_available, NULL, 0);
> 
> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Jonathan Cameron]

On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
>Added space around(one on each side of) binary
>operator(-) as preferred according to kernel
>coding style.
>
>Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>

Take a closer look at that macro. It isn't doing what you think...  To give a hint, changing this breaks userspace.

Jonathan


>---
> drivers/staging/iio/adc/ad7192.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/staging/iio/adc/ad7192.c
>b/drivers/staging/iio/adc/ad7192.c
>index d11c6de..1aee662 100644
>--- a/drivers/staging/iio/adc/ad7192.c
>+++ b/drivers/staging/iio/adc/ad7192.c
>@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> }
> 
> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
>-			     in_voltage-voltage_scale_available,
>+			     in_voltage - voltage_scale_available,
> 			     0444, ad7192_show_scale_available, NULL, 0);
> 
> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Jonathan Cameron]

On Fri, 8 Sep 2017 07:29:06 +0100
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:

> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> >Added space around(one on each side of) binary
> >operator(-) as preferred according to kernel
> >coding style.
> >
> >Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>  
> 
> Take a closer look at that macro. It isn't doing what you think...  
> To give a hint, changing this breaks userspace.

Ok, I'm bored of this particular one coming up. When you have
worked out what is going on Himanshi, would you mind putting
together a patch adding a comment describing why it is a bad
idea to 'fix' this?  That would be a very useful patch as
far as I'm concerned :)

There aren't that many cases of this in IIO so adding a comment
on each of them is probably reasonable just to avoid wasting
people's time on fixing them! (I think we have had more than
5 such goes this year so far...)

Jonathan

> 
> Jonathan
> 
> 
> >---
> > drivers/staging/iio/adc/ad7192.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/drivers/staging/iio/adc/ad7192.c
> >b/drivers/staging/iio/adc/ad7192.c
> >index d11c6de..1aee662 100644
> >--- a/drivers/staging/iio/adc/ad7192.c
> >+++ b/drivers/staging/iio/adc/ad7192.c
> >@@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> > }
> > 
> > static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> >-			     in_voltage-voltage_scale_available,
> >+			     in_voltage - voltage_scale_available,
> > 			     0444, ad7192_show_scale_available, NULL, 0);
> > 
> > static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,  
> 

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Lars-Peter Clausen]

On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> On Fri, 8 Sep 2017 07:29:06 +0100
> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> 
>> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
>>> Added space around(one on each side of) binary
>>> operator(-) as preferred according to kernel
>>> coding style.
>>>
>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>  
>>
>> Take a closer look at that macro. It isn't doing what you think...  
>> To give a hint, changing this breaks userspace.
> 
> Ok, I'm bored of this particular one coming up. When you have
> worked out what is going on Himanshi, would you mind putting
> together a patch adding a comment describing why it is a bad
> idea to 'fix' this?  That would be a very useful patch as
> far as I'm concerned :)
> 
> There aren't that many cases of this in IIO so adding a comment
> on each of them is probably reasonable just to avoid wasting
> people's time on fixing them! (I think we have had more than
> 5 such goes this year so far...)
> 

I'd much rather fix this API so that you have to put "" around the name so
that it is clear that it is a string, rather than doing the implicit
conversion to string using preprocessor magic. Better to have a
self-documenting API then having to add a comment to each user of the API.

> Jonathan
> 
>>
>> Jonathan
>>
>>
>>> ---
>>> drivers/staging/iio/adc/ad7192.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/iio/adc/ad7192.c
>>> b/drivers/staging/iio/adc/ad7192.c
>>> index d11c6de..1aee662 100644
>>> --- a/drivers/staging/iio/adc/ad7192.c
>>> +++ b/drivers/staging/iio/adc/ad7192.c
>>> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
>>> }
>>>
>>> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
>>> -			     in_voltage-voltage_scale_available,
>>> +			     in_voltage - voltage_scale_available,
>>> 			     0444, ad7192_show_scale_available, NULL, 0);
>>>
>>> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,  
>>
> 

Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Jonathan Cameron]

On Fri, 8 Sep 2017 11:37:56 +0200
Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> > On Fri, 8 Sep 2017 07:29:06 +0100
> > Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> >   
> >> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:  
> >>> Added space around(one on each side of) binary
> >>> operator(-) as preferred according to kernel
> >>> coding style.
> >>>
> >>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>    
> >>
> >> Take a closer look at that macro. It isn't doing what you think...  
> >> To give a hint, changing this breaks userspace.  
> > 
> > Ok, I'm bored of this particular one coming up. When you have
> > worked out what is going on Himanshi, would you mind putting
> > together a patch adding a comment describing why it is a bad
> > idea to 'fix' this?  That would be a very useful patch as
> > far as I'm concerned :)
> > 
> > There aren't that many cases of this in IIO so adding a comment
> > on each of them is probably reasonable just to avoid wasting
> > people's time on fixing them! (I think we have had more than
> > 5 such goes this year so far...)
> >   
> 
> I'd much rather fix this API so that you have to put "" around the name so
> that it is clear that it is a string, rather than doing the implicit
> conversion to string using preprocessor magic. Better to have a
> self-documenting API then having to add a comment to each user of the API.

Good point.

So Himanshi, feel like taking this on?

Jonathan

> 
> > Jonathan
> >   
> >>
> >> Jonathan
> >>
> >>  
> >>> ---
> >>> drivers/staging/iio/adc/ad7192.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/staging/iio/adc/ad7192.c
> >>> b/drivers/staging/iio/adc/ad7192.c
> >>> index d11c6de..1aee662 100644
> >>> --- a/drivers/staging/iio/adc/ad7192.c
> >>> +++ b/drivers/staging/iio/adc/ad7192.c
> >>> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> >>> }
> >>>
> >>> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> >>> -			     in_voltage-voltage_scale_available,
> >>> +			     in_voltage - voltage_scale_available,
> >>> 			     0444, ad7192_show_scale_available, NULL, 0);
> >>>
> >>> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,    
> >>  
> >   
> 

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Julia Lawall]

On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:

> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> > On Fri, 8 Sep 2017 07:29:06 +0100
> > Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> >
> >> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> >>> Added space around(one on each side of) binary
> >>> operator(-) as preferred according to kernel
> >>> coding style.
> >>>
> >>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> >>
> >> Take a closer look at that macro. It isn't doing what you think...
> >> To give a hint, changing this breaks userspace.
> >
> > Ok, I'm bored of this particular one coming up. When you have
> > worked out what is going on Himanshi, would you mind putting
> > together a patch adding a comment describing why it is a bad
> > idea to 'fix' this?  That would be a very useful patch as
> > far as I'm concerned :)
> >
> > There aren't that many cases of this in IIO so adding a comment
> > on each of them is probably reasonable just to avoid wasting
> > people's time on fixing them! (I think we have had more than
> > 5 such goes this year so far...)
> >
>
> I'd much rather fix this API so that you have to put "" around the name so
> that it is clear that it is a string, rather than doing the implicit
> conversion to string using preprocessor magic. Better to have a
> self-documenting API then having to add a comment to each user of the API.

All the DEVICE_ATTR macros use the same strategy.  And the non-IIO ones,
eg DEVICE_ATTR_RW, also use the provided name to construct the names of
the show and store functions, so I don't think a string would be
appropriate.

julia

>
> > Jonathan
> >
> >>
> >> Jonathan
> >>
> >>
> >>> ---
> >>> drivers/staging/iio/adc/ad7192.c | 2 +-
> >>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/staging/iio/adc/ad7192.c
> >>> b/drivers/staging/iio/adc/ad7192.c
> >>> index d11c6de..1aee662 100644
> >>> --- a/drivers/staging/iio/adc/ad7192.c
> >>> +++ b/drivers/staging/iio/adc/ad7192.c
> >>> @@ -341,7 +341,7 @@ static int ad7192_setup(struct ad7192_state *st,
> >>> }
> >>>
> >>> static IIO_DEVICE_ATTR_NAMED(in_v_m_v_scale_available,
> >>> -			     in_voltage-voltage_scale_available,
> >>> +			     in_voltage - voltage_scale_available,
> >>> 			     0444, ad7192_show_scale_available, NULL, 0);
> >>>
> >>> static IIO_DEVICE_ATTR(in_voltage_scale_available, 0444,
> >>
> >
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/2e3476f0-9e7e-2701-220a-5fb178d68d2e%40metafoo.de.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Lars-Peter Clausen]

On 09/08/2017 11:59 AM, Julia Lawall wrote:
> 
> 
> On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> 
>> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
>>> On Fri, 8 Sep 2017 07:29:06 +0100
>>> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
>>>
>>>> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
>>>>> Added space around(one on each side of) binary
>>>>> operator(-) as preferred according to kernel
>>>>> coding style.
>>>>>
>>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
>>>>
>>>> Take a closer look at that macro. It isn't doing what you think...
>>>> To give a hint, changing this breaks userspace.
>>>
>>> Ok, I'm bored of this particular one coming up. When you have
>>> worked out what is going on Himanshi, would you mind putting
>>> together a patch adding a comment describing why it is a bad
>>> idea to 'fix' this?  That would be a very useful patch as
>>> far as I'm concerned :)
>>>
>>> There aren't that many cases of this in IIO so adding a comment
>>> on each of them is probably reasonable just to avoid wasting
>>> people's time on fixing them! (I think we have had more than
>>> 5 such goes this year so far...)
>>>
>>
>> I'd much rather fix this API so that you have to put "" around the name so
>> that it is clear that it is a string, rather than doing the implicit
>> conversion to string using preprocessor magic. Better to have a
>> self-documenting API then having to add a comment to each user of the API.
> 
> All the DEVICE_ATTR macros use the same strategy.  And the non-IIO ones,
> eg DEVICE_ATTR_RW, also use the provided name to construct the names of
> the show and store functions, so I don't think a string would be
> appropriate.

I'm only suggesting to use a string for the _NAMED macros where the name is
not used to construct the identifiers.

In the case where the name is used to construct the identifiers we don't
have the issue with false positives since the name must be a valid
identifier on its own.

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Julia Lawall]

On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:

> On 09/08/2017 11:59 AM, Julia Lawall wrote:
> >
> >
> > On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> >
> >> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> >>> On Fri, 8 Sep 2017 07:29:06 +0100
> >>> Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> >>>
> >>>> On 8 September 2017 05:47:52 BST, Himanshi Jain <himshijain.hj@gmail.com> wrote:
> >>>>> Added space around(one on each side of) binary
> >>>>> operator(-) as preferred according to kernel
> >>>>> coding style.
> >>>>>
> >>>>> Signed-off-by: Himanshi Jain <himshijain.hj@gmail.com>
> >>>>
> >>>> Take a closer look at that macro. It isn't doing what you think...
> >>>> To give a hint, changing this breaks userspace.
> >>>
> >>> Ok, I'm bored of this particular one coming up. When you have
> >>> worked out what is going on Himanshi, would you mind putting
> >>> together a patch adding a comment describing why it is a bad
> >>> idea to 'fix' this?  That would be a very useful patch as
> >>> far as I'm concerned :)
> >>>
> >>> There aren't that many cases of this in IIO so adding a comment
> >>> on each of them is probably reasonable just to avoid wasting
> >>> people's time on fixing them! (I think we have had more than
> >>> 5 such goes this year so far...)
> >>>
> >>
> >> I'd much rather fix this API so that you have to put "" around the name so
> >> that it is clear that it is a string, rather than doing the implicit
> >> conversion to string using preprocessor magic. Better to have a
> >> self-documenting API then having to add a comment to each user of the API.
> >
> > All the DEVICE_ATTR macros use the same strategy.  And the non-IIO ones,
> > eg DEVICE_ATTR_RW, also use the provided name to construct the names of
> > the show and store functions, so I don't think a string would be
> > appropriate.
>
> I'm only suggesting to use a string for the _NAMED macros where the name is
> not used to construct the identifiers.
>
> In the case where the name is used to construct the identifiers we don't
> have the issue with false positives since the name must be a valid
> identifier on its own.

OK, it looks like a good project :)

julia

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Julia Lawall]

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



On Fri, 8 Sep 2017, Himanshi Jain wrote:

>
> Yeah, I am so sorry. That was too silly of me to just follow the checkpatch
> warning to think of a string passed as two variables being operated. I would
> definitely like to work on this so that others do not fall in this trap.
>
>
> So what do you suggest me to proceed with?
>
>  1.
>
>     If it is about adding comments to the files with these warnings that
>     these should not be fixed, I could find two more instances for this
>     warning in drivers/staging/iio/adc/* itself.
>
>
> I have a doubt here, so I tried searching for the checkpatch warnings using:
>
> perl scripts/checkpatch.pl -f drivers/staging/iio/*
>
> and it gives ï¿œnothing but an error (diff: drivers/staging/iio/accel/null: No
> such file or directory) due to folders contained in this directory. Is there
> a better way to search that I am missing? While finally I just ran this
> script for each folder individually with grep error to find two more
> instances.
>
>
>  1.
>
>     Or if it about the API, I am not that clear about what will I have to do
>     since I am new to this but as far as I could explore ï¿œand follow the
>     tags from :
>
> drivers/staging/iio/adc/ad7192.c (line 343 - IIO_DEVICE_ATTR_NAMED) ￜ->
> include/linux/iio/sysfs.h (line 88 - IIO_ATTR) ->
> ï¿œinclude/linux/iio/sysfs.h(line 55 - __ATTR) -> include/linux/sysfs.h(line
> 101 - __stringify(_name)) - where the implicit conversion of _name to string
> is being handled.
>
> Would this be done by defining a MACRO ￜ- VERIFY_IS_STRING(name) like for
> mode ï¿œVERIFY_OCTAL_PERMISSIONS(perms) in the file ï¿œinclude/linux/kernel.h ?

I think that the idea would be to make the NAMED macro take a string as an
argument in that particular position.  Then you will have to check how the
definition of that macro can cope with receiving a string.  There is a
whole chain of macros involved.  You could perhaps just inline them into
this definition.  The other option would be to create string variants of
all of the other macros, but perhaps that would be overkill if there are
no other uses.

Then you will have to update all the uses of this macro so that they take
a string.

julia

>
>
>
> On Friday, September 8, 2017 at 4:03:27 PM UTC+5:30, Julia Lawall wrote:
>
>
>       On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
>
>       > On 09/08/2017 11:59 AM, Julia Lawall wrote:
>       > >
>       > >
>       > > On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
>       > >
>       > >> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
>       > >>> On Fri, 8 Sep 2017 07:29:06 +0100
>       > >>> Jonathan Cameron <ji...@jic23.retrosnub.co.uk> wrote:
>       > >>>
>       > >>>> On 8 September 2017 05:47:52 BST, Himanshi Jain
>       <himshi...@gmail.com> wrote:
>       > >>>>> Added space around(one on each side of) binary
>       > >>>>> operator(-) as preferred according to kernel
>       > >>>>> coding style.
>       > >>>>>
>       > >>>>> Signed-off-by: Himanshi Jain <himshi...@gmail.com>
>       > >>>>
>       > >>>> Take a closer look at that macro. It isn't doing what you
>       think...
>       > >>>> To give a hint, changing this breaks userspace.
>       > >>>
>       > >>> Ok, I'm bored of this particular one coming up. When you
>       have
>       > >>> worked out what is going on Himanshi, would you mind
>       putting
>       > >>> together a patch adding a comment describing why it is a
>       bad
>       > >>> idea to 'fix' this? ï¿œThat would be a very useful patch as
>       > >>> far as I'm concerned :)
>       > >>>
>       > >>> There aren't that many cases of this in IIO so adding a
>       comment
>       > >>> on each of them is probably reasonable just to avoid
>       wasting
>       > >>> people's time on fixing them! (I think we have had more
>       than
>       > >>> 5 such goes this year so far...)
>       > >>>
>       > >>
>       > >> I'd much rather fix this API so that you have to put ""
>       around the name so
>       > >> that it is clear that it is a string, rather than doing the
>       implicit
>       > >> conversion to string using preprocessor magic. Better to
>       have a
>       > >> self-documenting API then having to add a comment to each
>       user of the API.
>       > >
>       > > All the DEVICE_ATTR macros use the same strategy. ï¿œAnd the
>       non-IIO ones,
>       > > eg DEVICE_ATTR_RW, also use the provided name to construct
>       the names of
>       > > the show and store functions, so I don't think a string
>       would be
>       > > appropriate.
>       >
>       > I'm only suggesting to use a string for the _NAMED macros
>       where the name is
>       > not used to construct the identifiers.
>       >
>       > In the case where the name is used to construct the
>       identifiers we don't
>       > have the issue with false positives since the name must be a
>       valid
>       > identifier on its own.
>
>       OK, it looks like a good project :)
>
>       julia
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/bd51f23b-a84d-460d-9a7c-
> 7b3b3213def7%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Himanshi Jain]

So I would definitely like to work on this.
What do you suggest me to proceed with? Adding comments to the files
having this warning or modifying each MACRO use to pass string to it
(with double quotes) and modifying the MACRO to cope with receiving a
string(not sure how, would want to discuss and propose a solution in
case we want to work with this option).

On Fri, Sep 8, 2017 at 5:33 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
>
> On Fri, 8 Sep 2017, Himanshi Jain wrote:
>
> >
> > Yeah, I am so sorry. That was too silly of me to just follow the checkpatch
> > warning to think of a string passed as two variables being operated. I would
> > definitely like to work on this so that others do not fall in this trap.
> >
> >
> > So what do you suggest me to proceed with?
> >
> >  1.
> >
> >     If it is about adding comments to the files with these warnings that
> >     these should not be fixed, I could find two more instances for this
> >     warning in drivers/staging/iio/adc/* itself.
> >
> >
> > I have a doubt here, so I tried searching for the checkpatch warnings using:
> >
> > perl scripts/checkpatch.pl -f drivers/staging/iio/*
> >
> > and it gives  nothing but an error (diff: drivers/staging/iio/accel/null: No
> > such file or directory) due to folders contained in this directory. Is there
> > a better way to search that I am missing? While finally I just ran this
> > script for each folder individually with grep error to find two more
> > instances.
> >
> >
> >  1.
> >
> >     Or if it about the API, I am not that clear about what will I have to do
> >     since I am new to this but as far as I could explore  and follow the
> >     tags from :
> >
> > drivers/staging/iio/adc/ad7192.c (line 343 - IIO_DEVICE_ATTR_NAMED)  ->
> > include/linux/iio/sysfs.h (line 88 - IIO_ATTR) ->
> >  include/linux/iio/sysfs.h(line 55 - __ATTR) -> include/linux/sysfs.h(line
> > 101 - __stringify(_name)) - where the implicit conversion of _name to string
> > is being handled.
> >
> > Would this be done by defining a MACRO  - VERIFY_IS_STRING(name) like for
> > mode  VERIFY_OCTAL_PERMISSIONS(perms) in the file  include/linux/kernel.h ?
>
> I think that the idea would be to make the NAMED macro take a string as an
> argument in that particular position.  Then you will have to check how the
> definition of that macro can cope with receiving a string.  There is a
> whole chain of macros involved.  You could perhaps just inline them into
> this definition.  The other option would be to create string variants of
> all of the other macros, but perhaps that would be overkill if there are
> no other uses.
>
> Then you will have to update all the uses of this macro so that they take
> a string.
>
> julia
>
> >
> >
> >
> > On Friday, September 8, 2017 at 4:03:27 PM UTC+5:30, Julia Lawall wrote:
> >
> >
> >       On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> >
> >       > On 09/08/2017 11:59 AM, Julia Lawall wrote:
> >       > >
> >       > >
> >       > > On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> >       > >
> >       > >> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> >       > >>> On Fri, 8 Sep 2017 07:29:06 +0100
> >       > >>> Jonathan Cameron <ji...@jic23.retrosnub.co.uk> wrote:
> >       > >>>
> >       > >>>> On 8 September 2017 05:47:52 BST, Himanshi Jain
> >       <himshi...@gmail.com> wrote:
> >       > >>>>> Added space around(one on each side of) binary
> >       > >>>>> operator(-) as preferred according to kernel
> >       > >>>>> coding style.
> >       > >>>>>
> >       > >>>>> Signed-off-by: Himanshi Jain <himshi...@gmail.com>
> >       > >>>>
> >       > >>>> Take a closer look at that macro. It isn't doing what you
> >       think...
> >       > >>>> To give a hint, changing this breaks userspace.
> >       > >>>
> >       > >>> Ok, I'm bored of this particular one coming up. When you
> >       have
> >       > >>> worked out what is going on Himanshi, would you mind
> >       putting
> >       > >>> together a patch adding a comment describing why it is a
> >       bad
> >       > >>> idea to 'fix' this?  That would be a very useful patch as
> >       > >>> far as I'm concerned :)
> >       > >>>
> >       > >>> There aren't that many cases of this in IIO so adding a
> >       comment
> >       > >>> on each of them is probably reasonable just to avoid
> >       wasting
> >       > >>> people's time on fixing them! (I think we have had more
> >       than
> >       > >>> 5 such goes this year so far...)
> >       > >>>
> >       > >>
> >       > >> I'd much rather fix this API so that you have to put ""
> >       around the name so
> >       > >> that it is clear that it is a string, rather than doing the
> >       implicit
> >       > >> conversion to string using preprocessor magic. Better to
> >       have a
> >       > >> self-documenting API then having to add a comment to each
> >       user of the API.
> >       > >
> >       > > All the DEVICE_ATTR macros use the same strategy.  And the
> >       non-IIO ones,
> >       > > eg DEVICE_ATTR_RW, also use the provided name to construct
> >       the names of
> >       > > the show and store functions, so I don't think a string
> >       would be
> >       > > appropriate.
> >       >
> >       > I'm only suggesting to use a string for the _NAMED macros
> >       where the name is
> >       > not used to construct the identifiers.
> >       >
> >       > In the case where the name is used to construct the
> >       identifiers we don't
> >       > have the issue with false positives since the name must be a
> >       valid
> >       > identifier on its own.
> >
> >       OK, it looks like a good project :)
> >
> >       julia
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/bd51f23b-a84d-460d-9a7c-
> > 7b3b3213def7%40googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.
> >
> >

Re: [Outreachy kernel] Re: [PATCH] Staging: iio: adc: Added Space around binary op.

[Julia Lawall]

On Sat, 9 Sep 2017, Himanshi Jain wrote:

> So I would definitely like to work on this.
> What do you suggest me to proceed with? Adding comments to the files
> having this warning or modifying each MACRO use to pass string to it
> (with double quotes) and modifying the MACRO to cope with receiving a
> string(not sure how, would want to discuss and propose a solution in
> case we want to work with this option).

Lars suggested to change the definition to take a string.  I think that if
you start looking at the code and unfolding the definitions, then it will
be clear what to do.

julia

>
> On Fri, Sep 8, 2017 at 5:33 PM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >
> >
> >
> > On Fri, 8 Sep 2017, Himanshi Jain wrote:
> >
> > >
> > > Yeah, I am so sorry. That was too silly of me to just follow the checkpatch
> > > warning to think of a string passed as two variables being operated. I would
> > > definitely like to work on this so that others do not fall in this trap.
> > >
> > >
> > > So what do you suggest me to proceed with?
> > >
> > >  1.
> > >
> > >     If it is about adding comments to the files with these warnings that
> > >     these should not be fixed, I could find two more instances for this
> > >     warning in drivers/staging/iio/adc/* itself.
> > >
> > >
> > > I have a doubt here, so I tried searching for the checkpatch warnings using:
> > >
> > > perl scripts/checkpatch.pl -f drivers/staging/iio/*
> > >
> > > and it gives  nothing but an error (diff: drivers/staging/iio/accel/null: No
> > > such file or directory) due to folders contained in this directory. Is there
> > > a better way to search that I am missing? While finally I just ran this
> > > script for each folder individually with grep error to find two more
> > > instances.
> > >
> > >
> > >  1.
> > >
> > >     Or if it about the API, I am not that clear about what will I have to do
> > >     since I am new to this but as far as I could explore  and follow the
> > >     tags from :
> > >
> > > drivers/staging/iio/adc/ad7192.c (line 343 - IIO_DEVICE_ATTR_NAMED)  ->
> > > include/linux/iio/sysfs.h (line 88 - IIO_ATTR) ->
> > >  include/linux/iio/sysfs.h(line 55 - __ATTR) -> include/linux/sysfs.h(line
> > > 101 - __stringify(_name)) - where the implicit conversion of _name to string
> > > is being handled.
> > >
> > > Would this be done by defining a MACRO  - VERIFY_IS_STRING(name) like for
> > > mode  VERIFY_OCTAL_PERMISSIONS(perms) in the file  include/linux/kernel.h ?
> >
> > I think that the idea would be to make the NAMED macro take a string as an
> > argument in that particular position.  Then you will have to check how the
> > definition of that macro can cope with receiving a string.  There is a
> > whole chain of macros involved.  You could perhaps just inline them into
> > this definition.  The other option would be to create string variants of
> > all of the other macros, but perhaps that would be overkill if there are
> > no other uses.
> >
> > Then you will have to update all the uses of this macro so that they take
> > a string.
> >
> > julia
> >
> > >
> > >
> > >
> > > On Friday, September 8, 2017 at 4:03:27 PM UTC+5:30, Julia Lawall wrote:
> > >
> > >
> > >       On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> > >
> > >       > On 09/08/2017 11:59 AM, Julia Lawall wrote:
> > >       > >
> > >       > >
> > >       > > On Fri, 8 Sep 2017, Lars-Peter Clausen wrote:
> > >       > >
> > >       > >> On 09/08/2017 11:32 AM, Jonathan Cameron wrote:
> > >       > >>> On Fri, 8 Sep 2017 07:29:06 +0100
> > >       > >>> Jonathan Cameron <ji...@jic23.retrosnub.co.uk> wrote:
> > >       > >>>
> > >       > >>>> On 8 September 2017 05:47:52 BST, Himanshi Jain
> > >       <himshi...@gmail.com> wrote:
> > >       > >>>>> Added space around(one on each side of) binary
> > >       > >>>>> operator(-) as preferred according to kernel
> > >       > >>>>> coding style.
> > >       > >>>>>
> > >       > >>>>> Signed-off-by: Himanshi Jain <himshi...@gmail.com>
> > >       > >>>>
> > >       > >>>> Take a closer look at that macro. It isn't doing what you
> > >       think...
> > >       > >>>> To give a hint, changing this breaks userspace.
> > >       > >>>
> > >       > >>> Ok, I'm bored of this particular one coming up. When you
> > >       have
> > >       > >>> worked out what is going on Himanshi, would you mind
> > >       putting
> > >       > >>> together a patch adding a comment describing why it is a
> > >       bad
> > >       > >>> idea to 'fix' this?  That would be a very useful patch as
> > >       > >>> far as I'm concerned :)
> > >       > >>>
> > >       > >>> There aren't that many cases of this in IIO so adding a
> > >       comment
> > >       > >>> on each of them is probably reasonable just to avoid
> > >       wasting
> > >       > >>> people's time on fixing them! (I think we have had more
> > >       than
> > >       > >>> 5 such goes this year so far...)
> > >       > >>>
> > >       > >>
> > >       > >> I'd much rather fix this API so that you have to put ""
> > >       around the name so
> > >       > >> that it is clear that it is a string, rather than doing the
> > >       implicit
> > >       > >> conversion to string using preprocessor magic. Better to
> > >       have a
> > >       > >> self-documenting API then having to add a comment to each
> > >       user of the API.
> > >       > >
> > >       > > All the DEVICE_ATTR macros use the same strategy.  And the
> > >       non-IIO ones,
> > >       > > eg DEVICE_ATTR_RW, also use the provided name to construct
> > >       the names of
> > >       > > the show and store functions, so I don't think a string
> > >       would be
> > >       > > appropriate.
> > >       >
> > >       > I'm only suggesting to use a string for the _NAMED macros
> > >       where the name is
> > >       > not used to construct the identifiers.
> > >       >
> > >       > In the case where the name is used to construct the
> > >       identifiers we don't
> > >       > have the issue with false positives since the name must be a
> > >       valid
> > >       > identifier on its own.
> > >
> > >       OK, it looks like a good project :)
> > >
> > >       julia
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "outreachy-kernel" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > > email to outreachy-kernel+unsubscribe@googlegroups.com.
> > > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > > To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/bd51f23b-a84d-460d-9a7c-
> > > 7b3b3213def7%40googlegroups.com.
> > > For more options, visit https://groups.google.com/d/optout.
> > >
> > >
>