[PATCH] remove untouched code in kfifo_in

[PATCH] remove untouched code in kfifo_in

[Wei Yang]

In kfifo_in marco, one piece of code is arounded by if(0). This code in
introduced by Stefani Seibold <stefani@seibold.net> to suppress a compiler
warning. This warning is not there with the upgrade of gcc version. 

This patch just remove this code.
---
 include/linux/kfifo.h |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
index 10308c6..e7015bb 100644
--- a/include/linux/kfifo.h
+++ b/include/linux/kfifo.h
@@ -512,10 +512,6 @@ __kfifo_uint_must_check_helper( \
 	unsigned long __n = (n); \
 	const size_t __recsize = sizeof(*__tmp->rectype); \
 	struct __kfifo *__kfifo = &__tmp->kfifo; \
-	if (0) { \
-		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
-		__dummy = (typeof(__buf))NULL; \
-	} \
 	(__recsize) ?\
 	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
 	__kfifo_in(__kfifo, __buf, __n); \
-- 
1.7.5.4

Re: [PATCH] remove untouched code in kfifo_in

[richard -rw- weinberger]

On Thu, Oct 18, 2012 at 3:59 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> In kfifo_in marco, one piece of code is arounded by if(0). This code in
> introduced by Stefani Seibold <stefani@seibold.net> to suppress a compiler
> warning. This warning is not there with the upgrade of gcc version.
>
> This patch just remove this code.

Are you sure?
This code fragment looks like a compiler bomb to detect type mismatch to me...

-- 
Thanks,
//richard

Re: [PATCH] remove untouched code in kfifo_in

[Stefani Seibold]

Am Freitag, den 19.10.2012, 00:37 +0200 schrieb richard -rw- weinberger:
> On Thu, Oct 18, 2012 at 3:59 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
> > In kfifo_in marco, one piece of code is arounded by if(0). This code in
> > introduced by Stefani Seibold <stefani@seibold.net> to suppress a compiler
> > warning. This warning is not there with the upgrade of gcc version.
> >
> > This patch just remove this code.
> 
> Are you sure?
> This code fragment looks like a compiler bomb to detect type mismatch to me...
> 

Yes, you are great! That was the reason why i made this peace of code.
So don't remove it!

Re: [PATCH] remove untouched code in kfifo_in

[Jiri Kosina]

On Fri, 19 Oct 2012, Stefani Seibold wrote:

> > > In kfifo_in marco, one piece of code is arounded by if(0). This code in
> > > introduced by Stefani Seibold <stefani@seibold.net> to suppress a compiler
> > > warning. This warning is not there with the upgrade of gcc version.
> > >
> > > This patch just remove this code.
> > 
> > Are you sure?
> > This code fragment looks like a compiler bomb to detect type mismatch to me...
> 
> Yes, you are great! That was the reason why i made this peace of code.
> So don't remove it!

As even you, the author of the code, got confused by it for a while, 
adding an explanatory comment seems appropriate.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] remove untouched code in kfifo_in

[Andrew Morton]

On Fri, 19 Oct 2012 09:34:43 +0200 (CEST)
Jiri Kosina <jkosina@suse.cz> wrote:

> On Fri, 19 Oct 2012, Stefani Seibold wrote:
> 
> > > > In kfifo_in marco, one piece of code is arounded by if(0). This code in
> > > > introduced by Stefani Seibold <stefani@seibold.net> to suppress a compiler
> > > > warning. This warning is not there with the upgrade of gcc version.
> > > >
> > > > This patch just remove this code.
> > > 
> > > Are you sure?
> > > This code fragment looks like a compiler bomb to detect type mismatch to me...
> > 
> > Yes, you are great! That was the reason why i made this peace of code.
> > So don't remove it!
> 
> As even you, the author of the code, got confused by it for a while, 
> adding an explanatory comment seems appropriate.
> 

include/linux/typecheck.h?

Re: [PATCH] remove untouched code in kfifo_in

[richard -rw- weinberger]

On Tue, Oct 23, 2012 at 4:27 AM, Richard Yang
<weiyang@linux.vnet.ibm.com> wrote:
> On Mon, Oct 22, 2012 at 04:56:02PM -0700, Andrew Morton wrote:
>>On Fri, 19 Oct 2012 09:34:43 +0200 (CEST)
>>Jiri Kosina <jkosina@suse.cz> wrote:
>>
>>> On Fri, 19 Oct 2012, Stefani Seibold wrote:
>>>
>>> > > > In kfifo_in marco, one piece of code is arounded by if(0). This code in
>>> > > > introduced by Stefani Seibold <stefani@seibold.net> to suppress a compiler
>>> > > > warning. This warning is not there with the upgrade of gcc version.
>>> > > >
>>> > > > This patch just remove this code.
>>> > >
>>> > > Are you sure?
>>> > > This code fragment looks like a compiler bomb to detect type mismatch to me...
>>> >
>>> > Yes, you are great! That was the reason why i made this peace of code.
>>> > So don't remove it!
>>>
>>> As even you, the author of the code, got confused by it for a while,
>>> adding an explanatory comment seems appropriate.
>>>
>>
>>include/linux/typecheck.h?
>
> Something like this?
>
> From f20c316bf87d5db0816a01705b7869526c0b2363 Mon Sep 17 00:00:00 2001
> From: Wei Yang <weiyang@linux.vnet.ibm.com>
> Date: Wed, 17 Oct 2012 16:45:49 +0800
> Subject: [PATCH] Replace the type check code with typecheck() in kfifo_in
>
> In kfifo_in marco, one piece of code which is arounded by if(0) will check the
> type of __tmp->ptr_const and __buf. If they are different type, there will
> output a warning during compiling. This piece of code is not self explaining
> and a little bit hard to understand.
>
> Based on Andrew Morton's suggestion, this patch replace this with typecheck()
> which will be easy to understand.
> ---
>  include/linux/kfifo.h |    5 +----
>  1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
> index 10308c6..b48fe71 100644
> --- a/include/linux/kfifo.h
> +++ b/include/linux/kfifo.h
> @@ -512,10 +512,7 @@ __kfifo_uint_must_check_helper( \
>         unsigned long __n = (n); \
>         const size_t __recsize = sizeof(*__tmp->rectype); \
>         struct __kfifo *__kfifo = &__tmp->kfifo; \
> -       if (0) { \
> -               typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
> -               __dummy = (typeof(__buf))NULL; \
> -       } \
> +       typecheck(typeof(__tmp->ptr_const), __buf);\
>         (__recsize) ?\
>         __kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
>         __kfifo_in(__kfifo, __buf, __n); \

What about all the other open coded type checks in that file?

-- 
Thanks,
//richard

Re: [PATCH] remove untouched code in kfifo_in

[Andrew Morton]

On Tue, 23 Oct 2012 21:25:46 +0200
richard -rw- weinberger <richard.weinberger@gmail.com> wrote:

> > Subject: [PATCH] Replace the type check code with typecheck() in kfifo_in
> >
> > In kfifo_in marco, one piece of code which is arounded by if(0) will check the
> > type of __tmp->ptr_const and __buf. If they are different type, there will
> > output a warning during compiling. This piece of code is not self explaining
> > and a little bit hard to understand.
> >
> > Based on Andrew Morton's suggestion, this patch replace this with typecheck()
> > which will be easy to understand.
> > ---
> >  include/linux/kfifo.h |    5 +----
> >  1 files changed, 1 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
> > index 10308c6..b48fe71 100644
> > --- a/include/linux/kfifo.h
> > +++ b/include/linux/kfifo.h
> > @@ -512,10 +512,7 @@ __kfifo_uint_must_check_helper( \
> >         unsigned long __n = (n); \
> >         const size_t __recsize = sizeof(*__tmp->rectype); \
> >         struct __kfifo *__kfifo = &__tmp->kfifo; \
> > -       if (0) { \
> > -               typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
> > -               __dummy = (typeof(__buf))NULL; \
> > -       } \
> > +       typecheck(typeof(__tmp->ptr_const), __buf);\
> >         (__recsize) ?\
> >         __kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
> >         __kfifo_in(__kfifo, __buf, __n); \
> 
> What about all the other open coded type checks in that file?

Yes, there are a lot.  Searching for "if (0)" finds them.

Re: [PATCH] remove untouched code in kfifo_in

[richard -rw- weinberger]

On Fri, Oct 19, 2012 at 8:18 AM, Richard Yang
<weiyang@linux.vnet.ibm.com> wrote:
> On Fri, Oct 19, 2012 at 07:23:09AM +0200, Stefani Seibold wrote:
>>
>>Am Freitag, den 19.10.2012, 00:37 +0200 schrieb richard -rw- weinberger:
>>> On Thu, Oct 18, 2012 at 3:59 PM, Wei Yang <weiyang@linux.vnet.ibm.com> wrote:
>>> > In kfifo_in marco, one piece of code is arounded by if(0). This code in
>>> > introduced by Stefani Seibold <stefani@seibold.net> to suppress a compiler
>>> > warning. This warning is not there with the upgrade of gcc version.
>>> >
>>> > This patch just remove this code.
>>>
>>> Are you sure?
>>> This code fragment looks like a compiler bomb to detect type mismatch to me...
>>>
>>
>>Yes, you are great! That was the reason why i made this peace of code.
>>So don't remove it!
> Thanks for your confirmation.
>
> Hmm, would you give me a hint on how it check the type mismatch?

Here a small hint:
---cut---
struct x {
};
struct y {
};

#define check(a, b) \
({ \
        if (0) { \
                typeof(a) __dummy __attribute__ ((unused)); \
                __dummy = (typeof(b))NULL; \
        } \
})

int main(int argc, char **argv)
{
        struct x _x;
        struct y _y;

        check(&_x, &_x); //okay
        check(&_x, &_y); //will explode with "assignment from
incompatible pointer type"

        return 0;
}

---cut---


-- 
Thanks,
//richard

Re: [PATCH] remove untouched code in kfifo_in

[richard -rw- weinberger]

On Fri, Oct 19, 2012 at 10:24 AM, Richard Yang
<weiyang@linux.vnet.ibm.com> wrote:
> got it, thanks~

BTW: The surrounding if (0) {} block could be removed in future.
I'm sure Stefani added it to suppress a compiler warning.
But the typeof() magic has to stay...

-- 
Thanks,
//richard

Re: [PATCH] remove untouched code in kfifo_in

[Jiri Kosina]

On Wed, 17 Oct 2012, Wei Yang wrote:

> In kfifo_in marco, one piece of code is arounded by if(0).
> 
> This patch just remove this code.
> ---
>  include/linux/kfifo.h |    4 ----
>  1 files changed, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
> index 10308c6..e7015bb 100644
> --- a/include/linux/kfifo.h
> +++ b/include/linux/kfifo.h
> @@ -512,10 +512,6 @@ __kfifo_uint_must_check_helper( \
>  	unsigned long __n = (n); \
>  	const size_t __recsize = sizeof(*__tmp->rectype); \
>  	struct __kfifo *__kfifo = &__tmp->kfifo; \
> -	if (0) { \
> -		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
> -		__dummy = (typeof(__buf))NULL; \
> -	} \
>  	(__recsize) ?\
>  	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
>  	__kfifo_in(__kfifo, __buf, __n); \

Frankly, I'd first like to understand what was the rationale for adding it 
at the first place. Adding Stefani and lkml to CC.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] remove untouched code in kfifo_in

[Jiri Kosina]

On Wed, 17 Oct 2012, Richard Yang wrote:

> >> This patch just remove this code.
> >> ---
> >>  include/linux/kfifo.h |    4 ----
> >>  1 files changed, 0 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
> >> index 10308c6..e7015bb 100644
> >> --- a/include/linux/kfifo.h
> >> +++ b/include/linux/kfifo.h
> >> @@ -512,10 +512,6 @@ __kfifo_uint_must_check_helper( \
> >>  	unsigned long __n = (n); \
> >>  	const size_t __recsize = sizeof(*__tmp->rectype); \
> >>  	struct __kfifo *__kfifo = &__tmp->kfifo; \
> >> -	if (0) { \
> >> -		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
> >> -		__dummy = (typeof(__buf))NULL; \
> >> -	} \
> >>  	(__recsize) ?\
> >>  	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
> >>  	__kfifo_in(__kfifo, __buf, __n); \
> >
> >Frankly, I'd first like to understand what was the rationale for adding it 
> >at the first place. Adding Stefani and lkml to CC.
> 
> Agree.
> 
> BTW, by git blame, I just see commit 2e956fb3 which is checked in by Stefani. 
> While looking in the diff, the kfifo_in() is already there. Looks like this
> commit move the definition from one file to another file. 
> 
> So I am not sure the if(0) code is added by Stefani. Any other method to trace
> the ealier history?

git blame -- include/linux/kfifo-new.h 2e956fb3~1

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH] remove untouched code in kfifo_in

[Stefani Seibold]

This was introduce by me to suppress a compiler warning, so don't remove
it. 

Am Mittwoch, den 17.10.2012, 16:05 +0200 schrieb Jiri Kosina:
> On Wed, 17 Oct 2012, Richard Yang wrote:
> 
> > >> This patch just remove this code.
> > >> ---
> > >>  include/linux/kfifo.h |    4 ----
> > >>  1 files changed, 0 insertions(+), 4 deletions(-)
> > >> 
> > >> diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
> > >> index 10308c6..e7015bb 100644
> > >> --- a/include/linux/kfifo.h
> > >> +++ b/include/linux/kfifo.h
> > >> @@ -512,10 +512,6 @@ __kfifo_uint_must_check_helper( \
> > >>  	unsigned long __n = (n); \
> > >>  	const size_t __recsize = sizeof(*__tmp->rectype); \
> > >>  	struct __kfifo *__kfifo = &__tmp->kfifo; \
> > >> -	if (0) { \
> > >> -		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
> > >> -		__dummy = (typeof(__buf))NULL; \
> > >> -	} \
> > >>  	(__recsize) ?\
> > >>  	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
> > >>  	__kfifo_in(__kfifo, __buf, __n); \
> > >
> > >Frankly, I'd first like to understand what was the rationale for adding it 
> > >at the first place. Adding Stefani and lkml to CC.
> > 
> > Agree.
> > 
> > BTW, by git blame, I just see commit 2e956fb3 which is checked in by Stefani. 
> > While looking in the diff, the kfifo_in() is already there. Looks like this
> > commit move the definition from one file to another file. 
> > 
> > So I am not sure the if(0) code is added by Stefani. Any other method to trace
> > the ealier history?
> 
> git blame -- include/linux/kfifo-new.h 2e956fb3~1
> 

Re: [PATCH] remove untouched code in kfifo_in

[Stefani Seibold]

Am Donnerstag, den 18.10.2012, 09:59 +0800 schrieb Richard Yang:
> On Wed, Oct 17, 2012 at 09:51:49PM +0200, Stefani Seibold wrote:
> >This was introduce by me to suppress a compiler warning, so don't remove
> >it. 
> Which warning? I compile by removing this, but not find warning.
> I compile it on x86_64 platform.
> >

Sorry, i can't remember, it is three years and two gcc major releases
ago.

> >Am Mittwoch, den 17.10.2012, 16:05 +0200 schrieb Jiri Kosina:
> >> On Wed, 17 Oct 2012, Richard Yang wrote:
> >> 
> >> > >> This patch just remove this code.
> >> > >> ---
> >> > >>  include/linux/kfifo.h |    4 ----
> >> > >>  1 files changed, 0 insertions(+), 4 deletions(-)
> >> > >> 
> >> > >> diff --git a/include/linux/kfifo.h b/include/linux/kfifo.h
> >> > >> index 10308c6..e7015bb 100644
> >> > >> --- a/include/linux/kfifo.h
> >> > >> +++ b/include/linux/kfifo.h
> >> > >> @@ -512,10 +512,6 @@ __kfifo_uint_must_check_helper( \
> >> > >>  	unsigned long __n = (n); \
> >> > >>  	const size_t __recsize = sizeof(*__tmp->rectype); \
> >> > >>  	struct __kfifo *__kfifo = &__tmp->kfifo; \
> >> > >> -	if (0) { \
> >> > >> -		typeof(__tmp->ptr_const) __dummy __attribute__ ((unused)); \
> >> > >> -		__dummy = (typeof(__buf))NULL; \
> >> > >> -	} \
> >> > >>  	(__recsize) ?\
> >> > >>  	__kfifo_in_r(__kfifo, __buf, __n, __recsize) : \
> >> > >>  	__kfifo_in(__kfifo, __buf, __n); \
> >> > >
> >> > >Frankly, I'd first like to understand what was the rationale for adding it 
> >> > >at the first place. Adding Stefani and lkml to CC.
> >> > 
> >> > Agree.
> >> > 
> >> > BTW, by git blame, I just see commit 2e956fb3 which is checked in by Stefani. 
> >> > While looking in the diff, the kfifo_in() is already there. Looks like this
> >> > commit move the definition from one file to another file. 
> >> > 
> >> > So I am not sure the if(0) code is added by Stefani. Any other method to trace
> >> > the ealier history?
> >> 
> >> git blame -- include/linux/kfifo-new.h 2e956fb3~1
> >> 
> >
> 

Re: [PATCH] remove untouched code in kfifo_in

[Jiri Kosina]

On Thu, 18 Oct 2012, Stefani Seibold wrote:

> > >This was introduce by me to suppress a compiler warning, so don't remove
> > >it. 
> > Which warning? I compile by removing this, but not find warning.
> > I compile it on x86_64 platform.
> > >
> 
> Sorry, i can't remember, it is three years and two gcc major releases
> ago.

Well, exactly. gcc that needs this workaround seems to be pretty broken, 
and I am not seeing any warnings either myself.

As it's quite a obscure and confusing construction, I'd not hesitate 
removing that.

-- 
Jiri Kosina
SUSE Labs