I see that there are functions like this which basically say:
return 1 if true else return 0. Is it worth cleaning them up? Or is there any reason why this convention is followed?
use bool as the return type. No reason for return type to be int.
Signed-off-by: Pranith Kumar <[email protected]>
---
include/linux/rwsem-spinlock.h | 2 +-
include/linux/rwsem.h | 2 +-
kernel/locking/rwsem-spinlock.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/rwsem-spinlock.h b/include/linux/rwsem-spinlock.h
index d5b13bc..9026d2a 100644
--- a/include/linux/rwsem-spinlock.h
+++ b/include/linux/rwsem-spinlock.h
@@ -39,7 +39,7 @@ extern int __down_write_trylock(struct rw_semaphore *sem);
extern void __up_read(struct rw_semaphore *sem);
extern void __up_write(struct rw_semaphore *sem);
extern void __downgrade_write(struct rw_semaphore *sem);
-extern int rwsem_is_locked(struct rw_semaphore *sem);
+extern bool rwsem_is_locked(struct rw_semaphore *sem);
#endif /* __KERNEL__ */
#endif /* _LINUX_RWSEM_SPINLOCK_H */
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 091d993..04faf87 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -49,7 +49,7 @@ extern struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem);
#include <asm/rwsem.h>
/* In all implementations count != 0 means locked */
-static inline int rwsem_is_locked(struct rw_semaphore *sem)
+static inline bool rwsem_is_locked(struct rw_semaphore *sem)
{
return sem->count != 0;
}
diff --git a/kernel/locking/rwsem-spinlock.c b/kernel/locking/rwsem-spinlock.c
index 9be8a91..7374139 100644
--- a/kernel/locking/rwsem-spinlock.c
+++ b/kernel/locking/rwsem-spinlock.c
@@ -20,7 +20,7 @@ struct rwsem_waiter {
enum rwsem_waiter_type type;
};
-int rwsem_is_locked(struct rw_semaphore *sem)
+bool rwsem_is_locked(struct rw_semaphore *sem)
{
int ret = 1;
unsigned long flags;
--
1.7.9.5
On Thu, Jun 05, 2014 at 04:49:37PM -0400, Pranith Kumar wrote:
> I see that there are functions like this which basically say:
>
> return 1 if true else return 0. Is it worth cleaning them up? Or is
> there any reason why this convention is followed?
Hysterical raisins, a lot of people learnt C before it grew bool,
including me.
> use bool as the return type. No reason for return type to be int.
>
> Signed-off-by: Pranith Kumar <[email protected]>
> ---
> diff --git a/kernel/locking/rwsem-spinlock.c b/kernel/locking/rwsem-spinlock.c
> index 9be8a91..7374139 100644
> --- a/kernel/locking/rwsem-spinlock.c
> +++ b/kernel/locking/rwsem-spinlock.c
> @@ -20,7 +20,7 @@ struct rwsem_waiter {
> enum rwsem_waiter_type type;
> };
>
> -int rwsem_is_locked(struct rw_semaphore *sem)
> +bool rwsem_is_locked(struct rw_semaphore *sem)
> {
> int ret = 1;
> unsigned long flags;
Now, see that's a half arsed change, if you change the function return
value, you should also change the value we actually return, @ret above
to bool, and you should then also change the values used to 'true' and
'false'.
Now in general, I don't particularly like such superfluous changes, so
unless you can show that GCC actually generates better code, I'd prefer
to keep things as they are.
On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <[email protected]> wrote:
>
> Now in general, I don't particularly like such superfluous changes, so
> unless you can show that GCC actually generates better code, I'd prefer
> to keep things as they are.
Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
use bool as the return type for rwsem_is_locked() instead of int
Signed-off-by: Pranith Kumar <[email protected]>
---
include/linux/rwsem-spinlock.h | 2 +-
include/linux/rwsem.h | 2 +-
kernel/locking/rwsem-spinlock.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/rwsem-spinlock.h b/include/linux/rwsem-spinlock.h
index d5b13bc..9026d2a 100644
--- a/include/linux/rwsem-spinlock.h
+++ b/include/linux/rwsem-spinlock.h
@@ -39,7 +39,7 @@ extern int __down_write_trylock(struct rw_semaphore *sem);
extern void __up_read(struct rw_semaphore *sem);
extern void __up_write(struct rw_semaphore *sem);
extern void __downgrade_write(struct rw_semaphore *sem);
-extern int rwsem_is_locked(struct rw_semaphore *sem);
+extern bool rwsem_is_locked(struct rw_semaphore *sem);
#endif /* __KERNEL__ */
#endif /* _LINUX_RWSEM_SPINLOCK_H */
diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
index 03f3b05..b056780 100644
--- a/include/linux/rwsem.h
+++ b/include/linux/rwsem.h
@@ -40,7 +40,7 @@ extern struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem);
#include <asm/rwsem.h>
/* In all implementations count != 0 means locked */
-static inline int rwsem_is_locked(struct rw_semaphore *sem)
+static inline bool rwsem_is_locked(struct rw_semaphore *sem)
{
return sem->count != 0;
}
diff --git a/kernel/locking/rwsem-spinlock.c b/kernel/locking/rwsem-spinlock.c
index 9be8a91..3f8adf8 100644
--- a/kernel/locking/rwsem-spinlock.c
+++ b/kernel/locking/rwsem-spinlock.c
@@ -20,9 +20,9 @@ struct rwsem_waiter {
enum rwsem_waiter_type type;
};
-int rwsem_is_locked(struct rw_semaphore *sem)
+bool rwsem_is_locked(struct rw_semaphore *sem)
{
- int ret = 1;
+ bool ret = true;
unsigned long flags;
if (raw_spin_trylock_irqsave(&sem->wait_lock, flags)) {
--
1.7.9.5
On Fri, Jun 06, 2014 at 01:53:01PM -0400, Pranith Kumar wrote:
> On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <[email protected]> wrote:
> >
> > Now in general, I don't particularly like such superfluous changes, so
> > unless you can show that GCC actually generates better code, I'd prefer
> > to keep things as they are.
>
> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
still 2 bytes, so sure.
Which gcc did you use and what arch did you build? That might be useful
info for the changelog.
On Fri, Jun 6, 2014 at 1:56 PM, Peter Zijlstra <[email protected]> wrote:
>> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>
> still 2 bytes, so sure.
>
> Which gcc did you use and what arch did you build? That might be useful
> info for the changelog.
I am using gcc (Ubuntu/Linaro 4.7.3-2ubuntu1~12.04) 4.7.3. I tested
this using ARCH=um with a Pentium processor to turn on
CONFIG_RWSEM_GENERIC_SPINLOCK.
--
Pranith
On 06/06/2014 01:53 PM, Pranith Kumar wrote:
> On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <[email protected]> wrote:
>>
>> Now in general, I don't particularly like such superfluous changes, so
>> unless you can show that GCC actually generates better code, I'd prefer
>> to keep things as they are.
>
> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>
> use bool as the return type for rwsem_is_locked() instead of int
>
> Signed-off-by: Pranith Kumar <[email protected]>
> ---
> include/linux/rwsem-spinlock.h | 2 +-
> include/linux/rwsem.h | 2 +-
> kernel/locking/rwsem-spinlock.c | 4 ++--
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/rwsem-spinlock.h b/include/linux/rwsem-spinlock.h
> index d5b13bc..9026d2a 100644
> --- a/include/linux/rwsem-spinlock.h
> +++ b/include/linux/rwsem-spinlock.h
> @@ -39,7 +39,7 @@ extern int __down_write_trylock(struct rw_semaphore *sem);
> extern void __up_read(struct rw_semaphore *sem);
> extern void __up_write(struct rw_semaphore *sem);
> extern void __downgrade_write(struct rw_semaphore *sem);
> -extern int rwsem_is_locked(struct rw_semaphore *sem);
> +extern bool rwsem_is_locked(struct rw_semaphore *sem);
>
> #endif /* __KERNEL__ */
> #endif /* _LINUX_RWSEM_SPINLOCK_H */
> diff --git a/include/linux/rwsem.h b/include/linux/rwsem.h
> index 03f3b05..b056780 100644
> --- a/include/linux/rwsem.h
> +++ b/include/linux/rwsem.h
> @@ -40,7 +40,7 @@ extern struct rw_semaphore *rwsem_downgrade_wake(struct rw_semaphore *sem);
> #include <asm/rwsem.h>
>
> /* In all implementations count != 0 means locked */
> -static inline int rwsem_is_locked(struct rw_semaphore *sem)
> +static inline bool rwsem_is_locked(struct rw_semaphore *sem)
> {
> return sem->count != 0;
> }
> diff --git a/kernel/locking/rwsem-spinlock.c b/kernel/locking/rwsem-spinlock.c
> index 9be8a91..3f8adf8 100644
> --- a/kernel/locking/rwsem-spinlock.c
> +++ b/kernel/locking/rwsem-spinlock.c
> @@ -20,9 +20,9 @@ struct rwsem_waiter {
> enum rwsem_waiter_type type;
> };
>
> -int rwsem_is_locked(struct rw_semaphore *sem)
> +bool rwsem_is_locked(struct rw_semaphore *sem)
> {
> - int ret = 1;
> + bool ret = true;
> unsigned long flags;
>
> if (raw_spin_trylock_irqsave(&sem->wait_lock, flags)) {
>
I observed one other user of rwsem_is_locked() in xfs, change accordingly
Signed-off-by: Pranith Kumar <[email protected]>
---
fs/xfs/xfs_inode.c | 2 +-
fs/xfs/xfs_inode.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 768087b..9047eda 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -285,7 +285,7 @@ xfs_ilock_demote(
}
#if defined(DEBUG) || defined(XFS_WARN)
-int
+bool
xfs_isilocked(
xfs_inode_t *ip,
uint lock_flags)
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f2fcde5..80649a1 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -348,7 +348,7 @@ void xfs_ilock(xfs_inode_t *, uint);
int xfs_ilock_nowait(xfs_inode_t *, uint);
void xfs_iunlock(xfs_inode_t *, uint);
void xfs_ilock_demote(xfs_inode_t *, uint);
-int xfs_isilocked(xfs_inode_t *, uint);
+bool xfs_isilocked(xfs_inode_t *, uint);
uint xfs_ilock_data_map_shared(struct xfs_inode *);
uint xfs_ilock_attr_map_shared(struct xfs_inode *);
int xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
--
1.7.9.5
On Fri, 2014-06-06 at 19:56 +0200, Peter Zijlstra wrote:
> On Fri, Jun 06, 2014 at 01:53:01PM -0400, Pranith Kumar wrote:
> > On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <[email protected]> wrote:
> > >
> > > Now in general, I don't particularly like such superfluous changes, so
> > > unless you can show that GCC actually generates better code, I'd prefer
> > > to keep things as they are.
> >
> > Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>
> still 2 bytes, so sure.
>
> Which gcc did you use and what arch did you build? That might be useful
> info for the changelog.
Yeah, please attach the output of 'size kernel/locking/rwsem.o' for both
before and after. I think there's similar opportunity in other locking
code as well.
On Fri, Jun 6, 2014 at 2:41 PM, Davidlohr Bueso <[email protected]> wrote:
> On Fri, 2014-06-06 at 19:56 +0200, Peter Zijlstra wrote:
>> On Fri, Jun 06, 2014 at 01:53:01PM -0400, Pranith Kumar wrote:
>> > On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <[email protected]> wrote:
>> > >
>> > > Now in general, I don't particularly like such superfluous changes, so
>> > > unless you can show that GCC actually generates better code, I'd prefer
>> > > to keep things as they are.
>> >
>> > Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>>
>> still 2 bytes, so sure.
>>
>> Which gcc did you use and what arch did you build? That might be useful
>> info for the changelog.
>
> Yeah, please attach the output of 'size kernel/locking/rwsem.o' for both
> before and after. I think there's similar opportunity in other locking
> code as well.
>
Ok. Here is the data.
size kernel/locking/rwsem-spinlock.o
Before change to bool:
text data bss dec hex
1336 0 0 1336 538
After change to bool:
text data bss dec hex
1334 0 0 1334 536
--
Pranith
On Fri, Jun 06, 2014 at 02:11:18PM -0400, Pranith Kumar wrote:
> On 06/06/2014 01:53 PM, Pranith Kumar wrote:
> > On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <[email protected]> wrote:
> >>
> >> Now in general, I don't particularly like such superfluous changes, so
> >> unless you can show that GCC actually generates better code, I'd prefer
> >> to keep things as they are.
> >
> > Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
> >
> > use bool as the return type for rwsem_is_locked() instead of int
> >
> > Signed-off-by: Pranith Kumar <[email protected]>
....
Makes sense to me.
> I observed one other user of rwsem_is_locked() in xfs, change accordingly
>
> Signed-off-by: Pranith Kumar <[email protected]>
> ---
> fs/xfs/xfs_inode.c | 2 +-
> fs/xfs/xfs_inode.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 768087b..9047eda 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -285,7 +285,7 @@ xfs_ilock_demote(
> }
>
> #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +bool
> xfs_isilocked(
> xfs_inode_t *ip,
> uint lock_flags)
If you are going to change the return type to bool, then you should
also remove the manual "!!" conversions to a boolean return and let
the compiler do it in the most optimal way.
Cheers,
Dave.
--
Dave Chinner
[email protected]
On 06/06/2014 08:18 PM, Dave Chinner wrote:
> On Fri, Jun 06, 2014 at 02:11:18PM -0400, Pranith Kumar wrote:
>> On 06/06/2014 01:53 PM, Pranith Kumar wrote:
>>> On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <[email protected]> wrote:
>>>>
>>>> Now in general, I don't particularly like such superfluous changes, so
>>>> unless you can show that GCC actually generates better code, I'd prefer
>>>> to keep things as they are.
>>>
>>> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>>>
>>> use bool as the return type for rwsem_is_locked() instead of int
>>>
>
> If you are going to change the return type to bool, then you should
> also remove the manual "!!" conversions to a boolean return and let
> the compiler do it in the most optimal way.
>
Agreed, please find patch below:
change return type to bool to follow rwsem_is_locked()
Signed-off-by: Pranith Kumar <[email protected]>
---
fs/xfs/xfs_inode.c | 8 ++++----
fs/xfs/xfs_inode.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index a6115fe..c02ac49 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -285,25 +285,25 @@ xfs_ilock_demote(
}
#if defined(DEBUG) || defined(XFS_WARN)
-int
+bool
xfs_isilocked(
xfs_inode_t *ip,
uint lock_flags)
{
if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
if (!(lock_flags & XFS_ILOCK_SHARED))
- return !!ip->i_lock.mr_writer;
+ return !(ip->i_lock.mr_writer == 0);
return rwsem_is_locked(&ip->i_lock.mr_lock);
}
if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
if (!(lock_flags & XFS_IOLOCK_SHARED))
- return !!ip->i_iolock.mr_writer;
+ return !(ip->i_iolock.mr_writer == 0);
return rwsem_is_locked(&ip->i_iolock.mr_lock);
}
ASSERT(0);
- return 0;
+ return false;
}
#endif
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f72bffa..efebed6 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -346,7 +346,7 @@ void xfs_ilock(xfs_inode_t *, uint);
int xfs_ilock_nowait(xfs_inode_t *, uint);
void xfs_iunlock(xfs_inode_t *, uint);
void xfs_ilock_demote(xfs_inode_t *, uint);
-int xfs_isilocked(xfs_inode_t *, uint);
+bool xfs_isilocked(xfs_inode_t *, uint);
uint xfs_ilock_data_map_shared(struct xfs_inode *);
uint xfs_ilock_attr_map_shared(struct xfs_inode *);
int xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
--
1.9.1
On 06/06/2014 08:59 PM, Pranith Kumar wrote:
> On 06/06/2014 08:18 PM, Dave Chinner wrote:
>> On Fri, Jun 06, 2014 at 02:11:18PM -0400, Pranith Kumar wrote:
>>> On 06/06/2014 01:53 PM, Pranith Kumar wrote:
>>>> On Fri, Jun 6, 2014 at 3:35 AM, Peter Zijlstra <[email protected]> wrote:
>>>>>
>>>>> Now in general, I don't particularly like such superfluous changes, so
>>>>> unless you can show that GCC actually generates better code, I'd prefer
>>>>> to keep things as they are.
>>>>
>>>> Fixed and checked the assembly. It saves us 2 bytes of code, not much. I am not sure if that is worth it :(
>>>>
>>>> use bool as the return type for rwsem_is_locked() instead of int
>>>>
>
>>
>> If you are going to change the return type to bool, then you should
>> also remove the manual "!!" conversions to a boolean return and let
>> the compiler do it in the most optimal way.
>>
>
> Agreed, please find patch below:
>
Simplify the "!!" condition. This is much simpler. :)
change return type to bool to follow rwsem_is_locked()
Signed-off-by: Pranith Kumar <[email protected]>
---
fs/xfs/xfs_inode.c | 8 ++++----
fs/xfs/xfs_inode.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index a6115fe..c02ac49 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -285,25 +285,25 @@ xfs_ilock_demote(
}
#if defined(DEBUG) || defined(XFS_WARN)
-int
+bool
xfs_isilocked(
xfs_inode_t *ip,
uint lock_flags)
{
if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
if (!(lock_flags & XFS_ILOCK_SHARED))
- return !!ip->i_lock.mr_writer;
+ return (ip->i_lock.mr_writer != 0);
return rwsem_is_locked(&ip->i_lock.mr_lock);
}
if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
if (!(lock_flags & XFS_IOLOCK_SHARED))
- return !!ip->i_iolock.mr_writer;
+ return (ip->i_iolock.mr_writer != 0);
return rwsem_is_locked(&ip->i_iolock.mr_lock);
}
ASSERT(0);
- return 0;
+ return false;
}
#endif
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f72bffa..efebed6 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -346,7 +346,7 @@ void xfs_ilock(xfs_inode_t *, uint);
int xfs_ilock_nowait(xfs_inode_t *, uint);
void xfs_iunlock(xfs_inode_t *, uint);
void xfs_ilock_demote(xfs_inode_t *, uint);
-int xfs_isilocked(xfs_inode_t *, uint);
+bool xfs_isilocked(xfs_inode_t *, uint);
uint xfs_ilock_data_map_shared(struct xfs_inode *);
uint xfs_ilock_attr_map_shared(struct xfs_inode *);
int xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
--
1.9.1
On Fri, 2014-06-06 at 21:41 -0400, Pranith Kumar wrote:
> On 06/06/2014 08:59 PM, Pranith Kumar wrote:
> > On 06/06/2014 08:18 PM, Dave Chinner wrote:
> >> If you are going to change the return type to bool, then you should
> >> also remove the manual "!!" conversions to a boolean return and let
> >> the compiler do it in the most optimal way.
> > Agreed, please find patch below:
> Simplify the "!!" condition. This is much simpler. :)
[]
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> @@ -285,25 +285,25 @@ xfs_ilock_demote(
> }
>
> #if defined(DEBUG) || defined(XFS_WARN)
> -int
> +bool
> xfs_isilocked(
> xfs_inode_t *ip,
> uint lock_flags)
> {
> if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> if (!(lock_flags & XFS_ILOCK_SHARED))
> - return !!ip->i_lock.mr_writer;
> + return (ip->i_lock.mr_writer != 0);
simpler still would be just removing the !! completely.
I presume in no case would it make an actual difference
in emitted code.
ie:
return ip->i_lock.mr_writer;
On Fri, Jun 06, 2014 at 07:39:30PM -0700, Joe Perches wrote:
> On Fri, 2014-06-06 at 21:41 -0400, Pranith Kumar wrote:
> > On 06/06/2014 08:59 PM, Pranith Kumar wrote:
> > > On 06/06/2014 08:18 PM, Dave Chinner wrote:
> > >> If you are going to change the return type to bool, then you should
> > >> also remove the manual "!!" conversions to a boolean return and let
> > >> the compiler do it in the most optimal way.
> > > Agreed, please find patch below:
> > Simplify the "!!" condition. This is much simpler. :)
> []
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
>
> > @@ -285,25 +285,25 @@ xfs_ilock_demote(
> > }
> >
> > #if defined(DEBUG) || defined(XFS_WARN)
> > -int
> > +bool
> > xfs_isilocked(
> > xfs_inode_t *ip,
> > uint lock_flags)
> > {
> > if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
> > if (!(lock_flags & XFS_ILOCK_SHARED))
> > - return !!ip->i_lock.mr_writer;
> > + return (ip->i_lock.mr_writer != 0);
>
> simpler still would be just removing the !! completely.
> I presume in no case would it make an actual difference
> in emitted code.
>
> ie:
> return ip->i_lock.mr_writer;
Yup, that's exactly what I meant. Casting to a bool type does all
the work of squashing all non-zero values to 1...
Cheers,
Dave.
--
Dave Chinner
[email protected]
On 06/07/2014 07:44 PM, Dave Chinner wrote:
> On Fri, Jun 06, 2014 at 07:39:30PM -0700, Joe Perches wrote:
>> On Fri, 2014-06-06 at 21:41 -0400, Pranith Kumar wrote:
>>> On 06/06/2014 08:59 PM, Pranith Kumar wrote:
>>>> On 06/06/2014 08:18 PM, Dave Chinner wrote:
>>>>> If you are going to change the return type to bool, then you should
>>>>> also remove the manual "!!" conversions to a boolean return and let
>>>>> the compiler do it in the most optimal way.
>> simpler still would be just removing the !! completely.
>> I presume in no case would it make an actual difference
>> in emitted code.
>>
>> ie:
>> return ip->i_lock.mr_writer;
>
> Yup, that's exactly what I meant. Casting to a bool type does all
> the work of squashing all non-zero values to 1...
>
change return type if xfs_isilocked() to bool to follow rwsem_is_locked()
Signed-off-by: Pranith Kumar <[email protected]>
---
fs/xfs/xfs_inode.c | 8 ++++----
fs/xfs/xfs_inode.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index a6115fe..c02ac49 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -285,25 +285,25 @@ xfs_ilock_demote(
}
#if defined(DEBUG) || defined(XFS_WARN)
-int
+bool
xfs_isilocked(
xfs_inode_t *ip,
uint lock_flags)
{
if (lock_flags & (XFS_ILOCK_EXCL|XFS_ILOCK_SHARED)) {
if (!(lock_flags & XFS_ILOCK_SHARED))
- return !!ip->i_lock.mr_writer;
+ return ip->i_lock.mr_writer;
return rwsem_is_locked(&ip->i_lock.mr_lock);
}
if (lock_flags & (XFS_IOLOCK_EXCL|XFS_IOLOCK_SHARED)) {
if (!(lock_flags & XFS_IOLOCK_SHARED))
- return !!ip->i_iolock.mr_writer;
+ return ip->i_iolock.mr_writer;
return rwsem_is_locked(&ip->i_iolock.mr_lock);
}
ASSERT(0);
- return 0;
+ return false;
}
#endif
diff --git a/fs/xfs/xfs_inode.h b/fs/xfs/xfs_inode.h
index f72bffa..efebed6 100644
--- a/fs/xfs/xfs_inode.h
+++ b/fs/xfs/xfs_inode.h
@@ -346,7 +346,7 @@ void xfs_ilock(xfs_inode_t *, uint);
int xfs_ilock_nowait(xfs_inode_t *, uint);
void xfs_iunlock(xfs_inode_t *, uint);
void xfs_ilock_demote(xfs_inode_t *, uint);
-int xfs_isilocked(xfs_inode_t *, uint);
+bool xfs_isilocked(xfs_inode_t *, uint);
uint xfs_ilock_data_map_shared(struct xfs_inode *);
uint xfs_ilock_attr_map_shared(struct xfs_inode *);
int xfs_ialloc(struct xfs_trans *, xfs_inode_t *, umode_t,
--
1.9.1