[PATCH] ipc/sem: Three function calls less in do_semtimedop()

[PATCH] ipc/sem: Three function calls less in do_semtimedop()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 6 Jul 2019 14:16:24 +0200

Avoid three function calls by using ternary operators instead of
conditional statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 ipc/sem.c | 25 ++++++++-----------------
 1 file changed, 8 insertions(+), 17 deletions(-)

diff --git a/ipc/sem.c b/ipc/sem.c
index 7da4504bcc7c..56ea549ac270 100644
--- a/ipc/sem.c
+++ b/ipc/sem.c
@@ -2122,27 +2122,18 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
 		int idx = array_index_nospec(sops->sem_num, sma->sem_nsems);
 		curr = &sma->sems[idx];

-		if (alter) {
-			if (sma->complex_count) {
-				list_add_tail(&queue.list,
-						&sma->pending_alter);
-			} else {
-
-				list_add_tail(&queue.list,
-						&curr->pending_alter);
-			}
-		} else {
-			list_add_tail(&queue.list, &curr->pending_const);
-		}
+		list_add_tail(&queue.list,
+			      alter
+			      ? (sma->complex_count
+				? &sma->pending_alter
+				: &curr->pending_alter)
+			      : &curr->pending_const);
 	} else {
 		if (!sma->complex_count)
 			merge_queues(sma);

-		if (alter)
-			list_add_tail(&queue.list, &sma->pending_alter);
-		else
-			list_add_tail(&queue.list, &sma->pending_const);
-
+		list_add_tail(&queue.list,
+			      alter ? &sma->pending_alter : &sma->pending_const);
 		sma->complex_count++;
 	}

--
2.22.0

Re: [PATCH] ipc/sem: Three function calls less in do_semtimedop()

[Colin Ian King]

On 06/07/2019 13:28, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 6 Jul 2019 14:16:24 +0200
> 
> Avoid three function calls by using ternary operators instead of
> conditional statements.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  ipc/sem.c | 25 ++++++++-----------------
>  1 file changed, 8 insertions(+), 17 deletions(-)
> 
> diff --git a/ipc/sem.c b/ipc/sem.c
> index 7da4504bcc7c..56ea549ac270 100644
> --- a/ipc/sem.c
> +++ b/ipc/sem.c
> @@ -2122,27 +2122,18 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
>  		int idx = array_index_nospec(sops->sem_num, sma->sem_nsems);
>  		curr = &sma->sems[idx];
> 
> -		if (alter) {
> -			if (sma->complex_count) {
> -				list_add_tail(&queue.list,
> -						&sma->pending_alter);
> -			} else {
> -
> -				list_add_tail(&queue.list,
> -						&curr->pending_alter);
> -			}
> -		} else {
> -			list_add_tail(&queue.list, &curr->pending_const);
> -		}
> +		list_add_tail(&queue.list,
> +			      alter
> +			      ? (sma->complex_count
> +				? &sma->pending_alter
> +				: &curr->pending_alter)
> +			      : &curr->pending_const);

Just no. This is making the code harder to comprehend with no advantage.
Compilers are smart, let the do the optimization work and keep code
simple for us mere mortals.

Colin

>  	} else {
>  		if (!sma->complex_count)
>  			merge_queues(sma);
> 
> -		if (alter)
> -			list_add_tail(&queue.list, &sma->pending_alter);
> -		else
> -			list_add_tail(&queue.list, &sma->pending_const);
> -
> +		list_add_tail(&queue.list,
> +			      alter ? &sma->pending_alter : &sma->pending_const);
>  		sma->complex_count++;
>  	}
> 
> --
> 2.22.0
> 

Re: [PATCH] ipc/sem: Three function calls less in do_semtimedop()

[Al Viro]

On Sat, Jul 06, 2019 at 09:13:46PM +0100, Colin Ian King wrote:
> On 06/07/2019 13:28, Markus Elfring wrote:
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Sat, 6 Jul 2019 14:16:24 +0200
> > 
> > Avoid three function calls by using ternary operators instead of
> > conditional statements.
> > 
> > This issue was detected by using the Coccinelle software.
> > 
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > ---
> >  ipc/sem.c | 25 ++++++++-----------------
> >  1 file changed, 8 insertions(+), 17 deletions(-)
> > 
> > diff --git a/ipc/sem.c b/ipc/sem.c
> > index 7da4504bcc7c..56ea549ac270 100644
> > --- a/ipc/sem.c
> > +++ b/ipc/sem.c
> > @@ -2122,27 +2122,18 @@ static long do_semtimedop(int semid, struct sembuf __user *tsops,
> >  		int idx = array_index_nospec(sops->sem_num, sma->sem_nsems);
> >  		curr = &sma->sems[idx];
> > 
> > -		if (alter) {
> > -			if (sma->complex_count) {
> > -				list_add_tail(&queue.list,
> > -						&sma->pending_alter);
> > -			} else {
> > -
> > -				list_add_tail(&queue.list,
> > -						&curr->pending_alter);
> > -			}
> > -		} else {
> > -			list_add_tail(&queue.list, &curr->pending_const);
> > -		}
> > +		list_add_tail(&queue.list,
> > +			      alter
> > +			      ? (sma->complex_count
> > +				? &sma->pending_alter
> > +				: &curr->pending_alter)
> > +			      : &curr->pending_const);
> 
> Just no. This is making the code harder to comprehend with no advantage.
> Compilers are smart, let the do the optimization work and keep code
> simple for us mere mortals.

If anything, that would've been better off as

		int idx = array_index_nospec(sops->sem_num, sma->sem_nsems);
		struct sem *curr = &sma->sems[idx];
		struct list_head *list;	/* which queue to sleep on */

		if (!alter)
			list = &curr->pending_const;
		else if (sma->complex_count)
			list = &sma->pending_alter;
		else
			list = &curr->pending_alter;

		list_add_tail(&queue.list, list);

perhaps with better identifier than 'list'.  This kind of ?: (ab)use makes
for unreadable code and more than makes up for "hey, we are adding to some
list in all those cases" extra information passed to readers...

Re: ipc/sem: Three function calls less in do_semtimedop()

[Markus Elfring]

>> +		list_add_tail(&queue.list,
>> +			      alter
>> +			      ? (sma->complex_count
>> +				? &sma->pending_alter
>> +				: &curr->pending_alter)
>> +			      : &curr->pending_const);
>
> Just no. This is making the code harder to comprehend

This can be according to your current view.


> with no advantage.

I propose to take additional aspects into account for the interpretation
of such source code.
The shown design direction can provide benefits which might get
a lower value for the software development attention so far.


>> +		list_add_tail(&queue.list,
>> +			      alter ? &sma->pending_alter : &sma->pending_const);

Can this code variant look more succinct?

Regards,
Markus