[PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Shivani Bhardwaj]

Convert the function max_u64() to macro because macros tend to be more
flexible than inline functions and here, the macro is going to be type
safe so there are not going to be any side effects.

Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
---
 drivers/staging/lustre/lustre/ldlm/interval_tree.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/lustre/lustre/ldlm/interval_tree.c b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
index a2ea8e5..52d0afd 100644
--- a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
+++ b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
@@ -42,6 +42,8 @@
 #include "../include/obd_support.h"
 #include "../include/interval_tree.h"
 
+#define max_u64(x, y) ((x > y) ? x : y)
+
 enum {
 	INTERVAL_RED = 0,
 	INTERVAL_BLACK = 1
@@ -96,11 +98,6 @@ static inline int extent_equal(struct interval_node_extent *e1,
 	return (e1->start == e2->start) && (e1->end == e2->end);
 }
 
-static inline __u64 max_u64(__u64 x, __u64 y)
-{
-	return x > y ? x : y;
-}
-
 static struct interval_node *interval_first(struct interval_node *node)
 {
 	if (!node)
-- 
2.1.0

Re: [Outreachy kernel] [PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Greg KH]

On Sun, Nov 01, 2015 at 12:15:44AM +0530, Shivani Bhardwaj wrote:
> Convert the function max_u64() to macro because macros tend to be more
> flexible than inline functions and here, the macro is going to be type
> safe so there are not going to be any side effects.
> 
> Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> ---
>  drivers/staging/lustre/lustre/ldlm/interval_tree.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/interval_tree.c b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> index a2ea8e5..52d0afd 100644
> --- a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> +++ b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> @@ -42,6 +42,8 @@
>  #include "../include/obd_support.h"
>  #include "../include/interval_tree.h"
>  
> +#define max_u64(x, y) ((x > y) ? x : y)

Ick, no, there is already a _correct_ macro in the kernel for this,
don't try to create another one that does not work properly for all
users.

And your patch is also backwards, inline functions are better than
macros, don't ever convert from an inline function to a macro.

thanks,

greg k-h

Re: [Outreachy kernel] [PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Shivani Bhardwaj]

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

On Sun, Nov 1, 2015 at 12:23 AM, Greg KH <gregkh@linuxfoundation.org> wrote:

> On Sun, Nov 01, 2015 at 12:15:44AM +0530, Shivani Bhardwaj wrote:
> > Convert the function max_u64() to macro because macros tend to be more
> > flexible than inline functions and here, the macro is going to be type
> > safe so there are not going to be any side effects.
> >
> > Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> > ---
> >  drivers/staging/lustre/lustre/ldlm/interval_tree.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> > index a2ea8e5..52d0afd 100644
> > --- a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> > +++ b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> > @@ -42,6 +42,8 @@
> >  #include "../include/obd_support.h"
> >  #include "../include/interval_tree.h"
> >
> > +#define max_u64(x, y) ((x > y) ? x : y)
>
> Ick, no, there is already a _correct_ macro in the kernel for this,
> don't try to create another one that does not work properly for all
> users.
>
> And your patch is also backwards, inline functions are better than
> macros, don't ever convert from an inline function to a macro.
>
>
Alright. Thanks for explaining.


> thanks,
>
> greg k-h
>

[-- Attachment #2: Type: text/html, Size: 2025 bytes --]

Re: [Outreachy kernel] [PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1847 bytes --]



On Sun, 1 Nov 2015, Shivani Bhardwaj wrote:

> 
> 
> On Sun, Nov 1, 2015 at 12:23 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
>       On Sun, Nov 01, 2015 at 12:15:44AM +0530, Shivani Bhardwaj
>       wrote:
>       > Convert the function max_u64() to macro because macros tend to
>       be more
>       > flexible than inline functions and here, the macro is going to
>       be type
>       > safe so there are not going to be any side effects.
>       >
>       > Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
>       > ---
>       >  drivers/staging/lustre/lustre/ldlm/interval_tree.c | 7
>       ++-----
>       >  1 file changed, 2 insertions(+), 5 deletions(-)
>       >
>       > diff --git
>       a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
>       b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
>       > index a2ea8e5..52d0afd 100644
>       > --- a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
>       > +++ b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
>       > @@ -42,6 +42,8 @@
>       >  #include "../include/obd_support.h"
>       >  #include "../include/interval_tree.h"
>       >
>       > +#define max_u64(x, y) ((x > y) ? x : y)
> 
>       Ick, no, there is already a _correct_ macro in the kernel for
>       this,
>       don't try to create another one that does not work properly for
>       all
>       users.
> 
>       And your patch is also backwards, inline functions are better
>       than
>       macros, don't ever convert from an inline function to a macro.
> 
> 
> Alright. Thanks for explaining.

Inline functons have types, which help you find bugs.

Inline functions don't risk duplicating computations.  In your macro, if 
eg x is the larger value, then whatever x is replaced by will be evaluated 
twice.  Consider what would happen if x is eg i++.

julia

Re: [Outreachy kernel] [PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Shivani Bhardwaj]

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

On Sun, Nov 1, 2015 at 12:39 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:

>
>
> On Sun, 1 Nov 2015, Shivani Bhardwaj wrote:
>
> >
> >
> > On Sun, Nov 1, 2015 at 12:23 AM, Greg KH <gregkh@linuxfoundation.org>
> wrote:
> >       On Sun, Nov 01, 2015 at 12:15:44AM +0530, Shivani Bhardwaj
> >       wrote:
> >       > Convert the function max_u64() to macro because macros tend to
> >       be more
> >       > flexible than inline functions and here, the macro is going to
> >       be type
> >       > safe so there are not going to be any side effects.
> >       >
> >       > Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
> >       > ---
> >       >  drivers/staging/lustre/lustre/ldlm/interval_tree.c | 7
> >       ++-----
> >       >  1 file changed, 2 insertions(+), 5 deletions(-)
> >       >
> >       > diff --git
> >       a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> >       b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> >       > index a2ea8e5..52d0afd 100644
> >       > --- a/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> >       > +++ b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
> >       > @@ -42,6 +42,8 @@
> >       >  #include "../include/obd_support.h"
> >       >  #include "../include/interval_tree.h"
> >       >
> >       > +#define max_u64(x, y) ((x > y) ? x : y)
> >
> >       Ick, no, there is already a _correct_ macro in the kernel for
> >       this,
> >       don't try to create another one that does not work properly for
> >       all
> >       users.
> >
> >       And your patch is also backwards, inline functions are better
> >       than
> >       macros, don't ever convert from an inline function to a macro.
> >
> >
> > Alright. Thanks for explaining.
>
> Inline functons have types, which help you find bugs.
>
> Inline functions don't risk duplicating computations.  In your macro, if
> eg x is the larger value, then whatever x is replaced by will be evaluated
> twice.  Consider what would happen if x is eg i++.
>
> Yes, Julia. I took into consideration this case  and I checked out the
entire code where the function was called, there was no situation like
this. Plus, I had read that inline functions are compiler dependent so I
thought of making the change.
Thank you for explaining. :)

julia

[-- Attachment #2: Type: text/html, Size: 3510 bytes --]

Re: [Outreachy kernel] [PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Julia Lawall]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1024 bytes --]

> Inline functons have types, which help you find bugs.
> 
> Inline functions don't risk duplicating computations.  In your macro,
> if
> eg x is the larger value, then whatever x is replaced by will be
> evaluated
> twice.  Consider what would happen if x is eg i++.
> 
> Yes, Julia. I took into consideration this case  and I checked out the
> entire code where the function was called, there was no situation like this.
> Plus, I had read that inline functions are compiler dependent so I thought
> of making the change.
> Thank you for explaining. :)

OK.  Still, someone else could extend the code in some way and want to use 
what looks like a function, and use is in a bad way.

Anyway, the best is always to find a standard kernel function that does 
the same thing.  There are lots of them for these kind of numerical 
things.

Also, you should set up your mailer so that it marks what you are replying 
to in some way.  In the above, one can't see the difference between what I 
wrote and what you wrote.

julia

Re: [Outreachy kernel] [PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Shivani Bhardwaj]

On Sun, Nov 1, 2015 at 12:45 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> Inline functons have types, which help you find bugs.
>>
>> Inline functions don't risk duplicating computations.  In your macro,
>> if
>> eg x is the larger value, then whatever x is replaced by will be
>> evaluated
>> twice.  Consider what would happen if x is eg i++.
>>
>> Yes, Julia. I took into consideration this case  and I checked out the
>> entire code where the function was called, there was no situation like this.
>> Plus, I had read that inline functions are compiler dependent so I thought
>> of making the change.
>> Thank you for explaining. :)
>
> OK.  Still, someone else could extend the code in some way and want to use
> what looks like a function, and use is in a bad way.
>
> Anyway, the best is always to find a standard kernel function that does
> the same thing.  There are lots of them for these kind of numerical
> things.
>
> Also, you should set up your mailer so that it marks what you are replying
> to in some way.  In the above, one can't see the difference between what I
> wrote and what you wrote.
>
> julia

Is it OK now?

Re: [Outreachy kernel] [PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Julia Lawall]

On Sun, 1 Nov 2015, Shivani Bhardwaj wrote:

> On Sun, Nov 1, 2015 at 12:45 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
> >> Inline functons have types, which help you find bugs.
> >>
> >> Inline functions don't risk duplicating computations.  In your macro,
> >> if
> >> eg x is the larger value, then whatever x is replaced by will be
> >> evaluated
> >> twice.  Consider what would happen if x is eg i++.
> >>
> >> Yes, Julia. I took into consideration this case  and I checked out the
> >> entire code where the function was called, there was no situation like this.
> >> Plus, I had read that inline functions are compiler dependent so I thought
> >> of making the change.
> >> Thank you for explaining. :)
> >
> > OK.  Still, someone else could extend the code in some way and want to use
> > what looks like a function, and use is in a bad way.
> >
> > Anyway, the best is always to find a standard kernel function that does
> > the same thing.  There are lots of them for these kind of numerical
> > things.
> >
> > Also, you should set up your mailer so that it marks what you are replying
> > to in some way.  In the above, one can't see the difference between what I
> > wrote and what you wrote.
> >
> > julia
> 
> Is it OK now?

Perfect. Thanks!

julia

Re: [Outreachy kernel] [PATCH] Staging: lustre: interval_tree: Convert inline function to macro

[Shivani Bhardwaj]

On Sun, Nov 1, 2015 at 1:06 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>
>
> On Sun, 1 Nov 2015, Shivani Bhardwaj wrote:
>
>> On Sun, Nov 1, 2015 at 12:45 AM, Julia Lawall <julia.lawall@lip6.fr> wrote:
>> >> Inline functons have types, which help you find bugs.
>> >>
>> >> Inline functions don't risk duplicating computations.  In your macro,
>> >> if
>> >> eg x is the larger value, then whatever x is replaced by will be
>> >> evaluated
>> >> twice.  Consider what would happen if x is eg i++.
>> >>
>> >> Yes, Julia. I took into consideration this case  and I checked out the
>> >> entire code where the function was called, there was no situation like this.
>> >> Plus, I had read that inline functions are compiler dependent so I thought
>> >> of making the change.
>> >> Thank you for explaining. :)
>> >
>> > OK.  Still, someone else could extend the code in some way and want to use
>> > what looks like a function, and use is in a bad way.
>> >
>> > Anyway, the best is always to find a standard kernel function that does
>> > the same thing.  There are lots of them for these kind of numerical
>> > things.
>> >
>> > Also, you should set up your mailer so that it marks what you are replying
>> > to in some way.  In the above, one can't see the difference between what I
>> > wrote and what you wrote.
>> >
>> > julia
>>
>> Is it OK now?
>
> Perfect. Thanks!
>
> julia

Thank you