Staging: lustre: interval_tree: Convert inline function to macro

PATCH In review Shivani Bhardwaj 3 participants 9 messages Oct 31, 2015 Staging

Share & tools

Shivani Bhardwaj

author

2015-10-31 18:45 UTC #0 lore raw

PATCH 1/1 Staging: lustre: interval_tree: Convert inline function to macro

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 <[email protected]>

---
 drivers/staging/lustre/lustre/ldlm/interval_tree.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

drivers/staging/lustre/lustre/ldlm/interval_tree.c +2 -5

index a2ea8e5..52d0afd 100644
@@ -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)

Signature · 1 line

2.1.0

Greg KH

maintainer

2015-10-31 18:53 UTC #1 lore raw

Quoted 14 lines · 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 <[email protected]>
---
 drivers/staging/lustre/lustre/ldlm/interval_tree.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/staging/lustre/lustre/ldlm/interval_tree.c +0 -0
index a2ea8e5..52d0afd 100644

drivers/staging/lustre/lustre/ldlm/interval_tree.c line 45

Quoted 4 lines

drivers/staging/lustre/lustre/ldlm/interval_tree.c +1 -0

@@ -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

Shivani Bhardwaj

author

2015-10-31 18:54 UTC #2 lore raw

Quoted 19 lines · Greg KH

Review
Quoted 10 lines · 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 <[email protected]>
---
 drivers/staging/lustre/lustre/ldlm/interval_tree.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
(quoted diff) +0 -0
b/drivers/staging/lustre/lustre/ldlm/interval_tree.c
Quoted 7 lines
index a2ea8e5..52d0afd 100644
drivers/staging/lustre/lustre/ldlm/interval_tree.c +0 -0
@@ -42,6 +42,8 @@
 #include "../include/obd_support.h"
 #include "../include/interval_tree.h"
 

Quoted 10 lines

drivers/staging/lustre/lustre/ldlm/interval_tree.c line 45
Quoted 1 line
drivers/staging/lustre/lustre/ldlm/interval_tree.c +1 -0
+#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.

Quoted 4 lines

thanks,
greg k-h

Julia Lawall

2015-10-31 19:09 UTC #3 lore raw

Quoted 35 lines · Shivani Bhardwaj

On Sun, Nov 1, 2015 at 12:23 AM, Greg KH <[email protected]> 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 <[email protected]> > ---
> 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.

Quoted 6 lines

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

Shivani Bhardwaj

author

2015-10-31 19:12 UTC #4 lore raw

Quoted 47 lines · Julia Lawall

Quoted 3 lines · Shivani Bhardwaj
On Sun, Nov 1, 2015 at 12:23 AM, Greg KH <[email protected]>
Quoted 32 lines · 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 <[email protected]> > ---
> 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.
Quoted 6 lines
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.

Quoted 7 lines

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

Julia Lawall

2015-10-31 19:15 UTC #5 lore raw

Quoted 8 lines

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++.

Quoted 5 lines

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

Shivani Bhardwaj

author

2015-10-31 19:27 UTC #6 lore raw

Quoted 21 lines · Julia Lawall

Quoted 8 lines
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++.
Quoted 5 lines
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.

Quoted 5 lines

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?

Julia Lawall

2015-10-31 19:36 UTC #7 lore raw

Quoted 22 lines · Shivani Bhardwaj

Quoted 21 lines · Julia Lawall
Quoted 8 lines
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++.
Quoted 5 lines
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.

Quoted 7 lines

Quoted 5 lines
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

Shivani Bhardwaj

author

2015-10-31 19:38 UTC #8 lore raw

Quoted 30 lines · Julia Lawall

Quoted 26 lines · Shivani Bhardwaj
Quoted 25 lines · Julia Lawall
Quoted 8 lines
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++.
Quoted 5 lines
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.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
julia
Is it OK now?
Perfect. Thanks!
julia

Thank you