[PATCH] mfd: asic3: One function call less in asic3_irq_probe()

[PATCH] mfd: asic3: One function call less in asic3_irq_probe()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 Jul 2019 20:22:26 +0200

Avoid an extra function call by using a ternary operator instead of
a conditional statement.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mfd/asic3.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c
index 83b18c998d6f..50f5368fb170 100644
--- a/drivers/mfd/asic3.c
+++ b/drivers/mfd/asic3.c
@@ -401,11 +401,10 @@ static int __init asic3_irq_probe(struct platform_device *pdev)
 	irq_base = asic->irq_base;

 	for (irq = irq_base; irq < irq_base + ASIC3_NR_IRQS; irq++) {
-		if (irq < asic->irq_base + ASIC3_NUM_GPIOS)
-			irq_set_chip(irq, &asic3_gpio_irq_chip);
-		else
-			irq_set_chip(irq, &asic3_irq_chip);
-
+		irq_set_chip(irq,
+			     (irq < asic->irq_base + ASIC3_NUM_GPIOS)
+			     ? &asic3_gpio_irq_chip
+			     : &asic3_irq_chip);
 		irq_set_chip_data(irq, asic);
 		irq_set_handler(irq, handle_level_irq);
 		irq_clear_status_flags(irq, IRQ_NOREQUEST | IRQ_NOPROBE);
--
2.22.0

Re: [PATCH] mfd: asic3: One function call less in asic3_irq_probe()

[Al Viro]

On Fri, Jul 05, 2019 at 08:30:08PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 5 Jul 2019 20:22:26 +0200
> 
> Avoid an extra function call by using a ternary operator instead of
> a conditional statement.

Which is a good thing, because...?

> This issue was detected by using the Coccinelle software.

Oh, I see - that answers all questions.  "Software has detected an issue",
so of course an issue it is.

> -		if (irq < asic->irq_base + ASIC3_NUM_GPIOS)
> -			irq_set_chip(irq, &asic3_gpio_irq_chip);
> -		else
> -			irq_set_chip(irq, &asic3_irq_chip);
> -
> +		irq_set_chip(irq,
> +			     (irq < asic->irq_base + ASIC3_NUM_GPIOS)
> +			     ? &asic3_gpio_irq_chip
> +			     : &asic3_irq_chip);

... except that the result is not objectively better by any real
criteria.  It's not more readable, it conveys _less_ information
to reader (the fact that calls differ only by the last argument
had been visually obvious already, and logics used to be easier
to see), it (obviously) does not generate better (or different)
code.  What the hell is the point?

May I politely inquire what makes you so determined to avoid any
not-entirely-mechanical activity?

Re: [PATCH] mfd: asic3: One function call less in asic3_irq_probe()

[Markus Elfring]

>> Avoid an extra function call by using a ternary operator instead of
>> a conditional statement.
>
> Which is a good thing, because...?

I suggest to reduce a bit of duplicate source code also at this place.


>> This issue was detected by using the Coccinelle software.
>
> Oh, I see - that answers all questions.

Obviously not so far.


> "Software has detected an issue", so of course an issue it is.

The mentioned development tool can help to point refactoring
possibilities out.


>> -		if (irq < asic->irq_base + ASIC3_NUM_GPIOS)
>> -			irq_set_chip(irq, &asic3_gpio_irq_chip);
>> -		else
>> -			irq_set_chip(irq, &asic3_irq_chip);
>> -
>> +		irq_set_chip(irq,
>> +			     (irq < asic->irq_base + ASIC3_NUM_GPIOS)
>> +			     ? &asic3_gpio_irq_chip
>> +			     : &asic3_irq_chip);
>
> ... except that the result is not objectively better by any real criteria.

We can have different opinions about the criteria which are relevant here.


> It's not more readable,

This is a possible view.


> it conveys _less_ information to reader

I guess that the interpretation of this feedback can become more interesting.


> (the fact that calls differ only by the last argument
> had been visually obvious already,

Can the repeated code specification make the recognition of this
implementation detail a bit harder actually?


> had been visually obvious already, and logics used to be easier
> to see), it (obviously) does not generate better (or different) code.

The functionality should be equivalent for the shown software refactoring.


> What the hell is the point?

I dare to point another change possibility out.
I am unsure if this adjustment will be picked up finally.

Regards,
Markus

Re: [PATCH] mfd: asic3: One function call less in asic3_irq_probe()

[Enrico Weigelt, metux IT consult]

On 07.07.19 09:56, Markus Elfring wrote:
>>> Avoid an extra function call by using a ternary operator instead of
>>> a conditional statement.
>>
>> Which is a good thing, because...?
> 
> I suggest to reduce a bit of duplicate source code also at this place.

Duplicate code (logic) or just characters ?

IMHO, readability is an important aspect, so we could be careful about
that. Some of your other patches IMHO made it actually a bit easier
to read, but this particular case doesnt seem so to (just according
to my personal taste).

I believe the compiler can do optimize that, based on the given flags.
(eg. size vs. speed). Therefore, I think that readability for the
human reader should be primary argument.

>> ... except that the result is not objectively better by any real criteria.
> 
> We can have different opinions about the criteria which are relevant here.

Which criterias are you operating on ?

> I dare to point another change possibility out.
> I am unsure if this adjustment will be picked up finally.

I think it's good that you're using tools like cocci for pointing out
*possible* points of useful refactoring. But that doesn't mean that a
particular patch can be accepted or not in the greater context.

Note that such issues are pretty subjective - it's not a technical but
an asthetic matter, so such issues can't be resolved by logic. Here, the
better something fits the personal taste of the maintainrs, the easier
it is for them to quickly understand the code (w/o having to give it any
deeper thoughts), thus reduces their brain load. Therefore that should
be the the primary argument left.

Don't see this as a judgment of your work as such - this kind of work
just tends to have a high rate of non-acceptable output (unless the
individual maintainer doing it himself).


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: mfd: asic3: One function call less in asic3_irq_probe()

[Markus Elfring]

>> I suggest to reduce a bit of duplicate source code also at this place.
>
> Duplicate code (logic) or just characters ?

Both.


> IMHO, readability is an important aspect, so we could be careful about that.

This is usual.

The code text size can influence this aspect in considerable ways.


>> We can have different opinions about the criteria which are relevant here.
>
> Which criterias are you operating on ?

I suggest occasionally again to reconsider consequences around a principle
like “Don't repeat yourself”.


> I think it's good that you're using tools like cocci for pointing out
> *possible* points of useful refactoring.

Thanks for your general understanding.


> But that doesn't mean that a particular patch can be accepted
> or not in the greater context.

Would you like to find the corresponding change acceptance out at all?


> Note that such issues are pretty subjective

The situation depends on some factors.


> - it's not a technical

I got an other impression.


> but an asthetic matter,

This matters also.


> so such issues can't be resolved by logic.

I guess that it can help.

Regards,
Markus

Re: mfd: asic3: One function call less in asic3_irq_probe()

[Enrico Weigelt, metux IT consult]

On 08.07.19 13:50, Markus Elfring wrote:
>>> I suggest to reduce a bit of duplicate source code also at this place.
>>
>> Duplicate code (logic) or just characters ?
> 
> Both.

You could also complexity for the human reader. That's a point where
personal perception comes in, which needs to be taken into account.

> The code text size can influence this aspect in considerable ways.

It *can*, but not necessarily. I'd prefer leaving this choice to the
maintainer, as he's the one who finally needs to care about all the
code. Discussions about such choices tend to be pointless (personal
perception / taste isn't something that can be debated by argument) and
risks annoying people and waste precious brain time.

> I suggest occasionally again to reconsider consequences around a principle
> like “Don't repeat yourself”.

In general correct. But in some cases repeating a few words can make the
code actually easier to read (psychologic phenomenon - human brains work
very different from computers). This needs to be carefully weighted.

Theorettically, we could do this conversation with much less words,
using some purely logic language, similar to math formulas - but for
most people this hard to do. Therefore, let's make the code easy for
us humans and let the machines (eg. compiler) do the heavy lifting.

>> But that doesn't mean that a particular patch can be accepted
>> or not in the greater context.
> 
> Would you like to find the corresponding change acceptance out at all?

For particular change, I wouldn't really object, but I don't like it
so much either. Some your other changes IMHO do make the code prettier.

>> - it's not a technical
> 
> I got an other impression.

That's the point. Psychology / personal perception plays a big role
here. We can't deduce it by pure logic.


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH] mfd: asic3: One function call less in asic3_irq_probe()

[Lee Jones]

On Fri, 05 Jul 2019, Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 5 Jul 2019 20:22:26 +0200
> 
> Avoid an extra function call by using a ternary operator instead of
> a conditional statement.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/mfd/asic3.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c
> index 83b18c998d6f..50f5368fb170 100644
> --- a/drivers/mfd/asic3.c
> +++ b/drivers/mfd/asic3.c
> @@ -401,11 +401,10 @@ static int __init asic3_irq_probe(struct platform_device *pdev)
>  	irq_base = asic->irq_base;
> 
>  	for (irq = irq_base; irq < irq_base + ASIC3_NR_IRQS; irq++) {
> -		if (irq < asic->irq_base + ASIC3_NUM_GPIOS)
> -			irq_set_chip(irq, &asic3_gpio_irq_chip);
> -		else
> -			irq_set_chip(irq, &asic3_irq_chip);
> -
> +		irq_set_chip(irq,
> +			     (irq < asic->irq_base + ASIC3_NUM_GPIOS)
> +			     ? &asic3_gpio_irq_chip
> +			     : &asic3_irq_chip);

The comparison better suits an if statement IMHO.

How about:

  		struct irq_chip *chip;

		if (irq < asic->irq_base + ASIC3_NUM_GPIOS)
			chip = &asic3_gpio_irq_chip;
		else
			chip = &asic3_irq_chip);

		irq_set_chip(irq, chip);

>  		irq_set_chip_data(irq, asic);
>  		irq_set_handler(irq, handle_level_irq);
>  		irq_clear_status_flags(irq, IRQ_NOREQUEST | IRQ_NOPROBE);

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH] mfd: asic3: One function call less in asic3_irq_probe()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 5 Jul 2019 20:22:26 +0200

Avoid an extra function call by using a ternary operator instead of
a conditional statement.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mfd/asic3.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/asic3.c b/drivers/mfd/asic3.c
index 83b18c998d6f..50f5368fb170 100644
--- a/drivers/mfd/asic3.c
+++ b/drivers/mfd/asic3.c
@@ -401,11 +401,10 @@ static int __init asic3_irq_probe(struct platform_device *pdev)
 	irq_base = asic->irq_base;

 	for (irq = irq_base; irq < irq_base + ASIC3_NR_IRQS; irq++) {
-		if (irq < asic->irq_base + ASIC3_NUM_GPIOS)
-			irq_set_chip(irq, &asic3_gpio_irq_chip);
-		else
-			irq_set_chip(irq, &asic3_irq_chip);
-
+		irq_set_chip(irq,
+			     (irq < asic->irq_base + ASIC3_NUM_GPIOS)
+			     ? &asic3_gpio_irq_chip
+			     : &asic3_irq_chip);
 		irq_set_chip_data(irq, asic);
 		irq_set_handler(irq, handle_level_irq);
 		irq_clear_status_flags(irq, IRQ_NOREQUEST | IRQ_NOPROBE);
--
2.22.0