* [PATCH] FDDI: defxx: simplify if-if to if-else
@ 2022-04-24 9:28 Wan Jiabing
2022-04-24 10:39 ` Maciej W. Rozycki
0 siblings, 1 reply; 6+ messages in thread
From: Wan Jiabing @ 2022-04-24 9:28 UTC (permalink / raw)
To: Maciej W. Rozycki, David S. Miller, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel
Cc: kael_w, Wan Jiabing
Use if and else instead of if(A) and if (!A).
Signed-off-by: Wan Jiabing <wanjiabing@vivo.com>
---
drivers/net/fddi/defxx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/fddi/defxx.c b/drivers/net/fddi/defxx.c
index b584ffe38ad6..3edb2e96f763 100644
--- a/drivers/net/fddi/defxx.c
+++ b/drivers/net/fddi/defxx.c
@@ -585,10 +585,10 @@ static int dfx_register(struct device *bdev)
bp->mmio = false;
dfx_get_bars(bp, bar_start, bar_len);
}
- }
- if (!dfx_use_mmio)
+ } else {
region = request_region(bar_start[0], bar_len[0],
bdev->driver->name);
+ }
if (!region) {
dfx_register_res_err(print_name, dfx_use_mmio,
bar_start[0], bar_len[0]);
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] FDDI: defxx: simplify if-if to if-else
2022-04-24 9:28 [PATCH] FDDI: defxx: simplify if-if to if-else Wan Jiabing
@ 2022-04-24 10:39 ` Maciej W. Rozycki
2022-04-24 22:17 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Maciej W. Rozycki @ 2022-04-24 10:39 UTC (permalink / raw)
To: Wan Jiabing
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, netdev,
linux-kernel, kael_w
On Sun, 24 Apr 2022, Wan Jiabing wrote:
> diff --git a/drivers/net/fddi/defxx.c b/drivers/net/fddi/defxx.c
> index b584ffe38ad6..3edb2e96f763 100644
> --- a/drivers/net/fddi/defxx.c
> +++ b/drivers/net/fddi/defxx.c
> @@ -585,10 +585,10 @@ static int dfx_register(struct device *bdev)
> bp->mmio = false;
> dfx_get_bars(bp, bar_start, bar_len);
> }
> - }
> - if (!dfx_use_mmio)
> + } else {
> region = request_region(bar_start[0], bar_len[0],
> bdev->driver->name);
> + }
NAK. The first conditional optionally sets `bp->mmio = false', which
changes the value of `dfx_use_mmio' in some configurations:
#if defined(CONFIG_EISA) || defined(CONFIG_PCI)
#define dfx_use_mmio bp->mmio
#else
#define dfx_use_mmio true
#endif
Maciej
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] FDDI: defxx: simplify if-if to if-else
2022-04-24 10:39 ` Maciej W. Rozycki
@ 2022-04-24 22:17 ` Andrew Lunn
2022-04-24 23:26 ` Maciej W. Rozycki
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2022-04-24 22:17 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Wan Jiabing, David S. Miller, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel, kael_w
On Sun, Apr 24, 2022 at 11:39:50AM +0100, Maciej W. Rozycki wrote:
> On Sun, 24 Apr 2022, Wan Jiabing wrote:
>
> > diff --git a/drivers/net/fddi/defxx.c b/drivers/net/fddi/defxx.c
> > index b584ffe38ad6..3edb2e96f763 100644
> > --- a/drivers/net/fddi/defxx.c
> > +++ b/drivers/net/fddi/defxx.c
> > @@ -585,10 +585,10 @@ static int dfx_register(struct device *bdev)
> > bp->mmio = false;
> > dfx_get_bars(bp, bar_start, bar_len);
> > }
> > - }
> > - if (!dfx_use_mmio)
> > + } else {
> > region = request_region(bar_start[0], bar_len[0],
> > bdev->driver->name);
> > + }
>
> NAK. The first conditional optionally sets `bp->mmio = false', which
> changes the value of `dfx_use_mmio' in some configurations:
>
> #if defined(CONFIG_EISA) || defined(CONFIG_PCI)
> #define dfx_use_mmio bp->mmio
> #else
> #define dfx_use_mmio true
> #endif
Which is just asking for trouble like this.
Could i suggest dfx_use_mmio is changed to DFX_USE_MMIO to give a hint
something horrible is going on.
It probably won't stop the robots finding this if (x) if (!x), but
there is a chance the robot drivers will wonder why it is upper case.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] FDDI: defxx: simplify if-if to if-else
2022-04-24 22:17 ` Andrew Lunn
@ 2022-04-24 23:26 ` Maciej W. Rozycki
2022-04-25 3:27 ` Jiabing Wan
2022-04-25 12:06 ` Andrew Lunn
0 siblings, 2 replies; 6+ messages in thread
From: Maciej W. Rozycki @ 2022-04-24 23:26 UTC (permalink / raw)
To: Andrew Lunn
Cc: Wan Jiabing, David S. Miller, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel, kael_w
On Mon, 25 Apr 2022, Andrew Lunn wrote:
> > NAK. The first conditional optionally sets `bp->mmio = false', which
> > changes the value of `dfx_use_mmio' in some configurations:
> >
> > #if defined(CONFIG_EISA) || defined(CONFIG_PCI)
> > #define dfx_use_mmio bp->mmio
> > #else
> > #define dfx_use_mmio true
> > #endif
>
> Which is just asking for trouble like this.
>
> Could i suggest dfx_use_mmio is changed to DFX_USE_MMIO to give a hint
> something horrible is going on.
There's legacy behind it, `dfx_use_mmio' used to be a proper variable and
references were retained not to obfuscate the changes that ultimately led
to the current arrangement. I guess at this stage it could be changed to
a function-like macro or a static inline function taking `bp' as the
argument.
> It probably won't stop the robots finding this if (x) if (!x), but
> there is a chance the robot drivers will wonder why it is upper case.
Well, blindly relying on automation is bound to cause trouble. There has
to be a piece of intelligence signing the results off at the end.
And there's nothing wrong with if (x) if (!x) in the first place; any
sane compiler will produce reasonable output from it. Don't fix what
ain't broke! And watch out for volatiles!
Maciej
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] FDDI: defxx: simplify if-if to if-else
2022-04-24 23:26 ` Maciej W. Rozycki
@ 2022-04-25 3:27 ` Jiabing Wan
2022-04-25 12:06 ` Andrew Lunn
1 sibling, 0 replies; 6+ messages in thread
From: Jiabing Wan @ 2022-04-25 3:27 UTC (permalink / raw)
To: Maciej W. Rozycki, Andrew Lunn
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, netdev,
linux-kernel, kael_w
On 2022/4/25 7:26, Maciej W. Rozycki wrote:
> On Mon, 25 Apr 2022, Andrew Lunn wrote:
>
>>> NAK. The first conditional optionally sets `bp->mmio = false', which
>>> changes the value of `dfx_use_mmio' in some configurations:
>>>
>>> #if defined(CONFIG_EISA) || defined(CONFIG_PCI)
>>> #define dfx_use_mmio bp->mmio
>>> #else
>>> #define dfx_use_mmio true
>>> #endif
Yes, it's my fault. I didn't notice "dfx_use_mmio" is a MACRO,
sorry for this wrong patch.
>> It probably won't stop the robots finding this if (x) if (!x), but
>> there is a chance the robot drivers will wonder why it is upper case.
> Well, blindly relying on automation is bound to cause trouble. There has
> to be a piece of intelligence signing the results off at the end.
You are right and I'll be more careful to review the result before
submitting.
>
> And there's nothing wrong with if (x) if (!x) in the first place; any
> sane compiler will produce reasonable output from it. Don't fix what
> ain't broke! And watch out for volatiles!
Yes, there's nothing wrong with if (x) if (!x), but I want to do is
reducing the complexity of the code.
There would be less instructions when using "if and else" rather
than "if (A) and if (!A)" as I tested:
Use if(A) and if(!A):
ldr w0, [sp, 28]
cmp w0, 0
beq .L2
ldr w0, [sp, 28]
add w0, w0, 1
str w0, [sp, 28]
.L2:
ldr w0, [sp, 28] <------ one more ldr instruction
cmp w0, 0 <------ one more cmp instruction
bne .L3
ldr w0, [sp, 28]
add w0, w0, 2
str w0, [sp, 28]
.L3:
ldr w0, [sp, 28]
mov w1, w0
adrp x0, .LC1
add x0, x0, :lo12:.LC1
bl printf
Use if(A) and else:
ldr w0, [sp, 28]
cmp w0, 0
beq .L2
ldr w0, [sp, 28]
add w0, w0, 1
str w0, [sp, 28] <------ reduce two instructions
b .L3
.L2:
ldr w0, [sp, 28]
add w0, w0, 2
str w0, [sp, 28]
.L3:
ldr w0, [sp, 28]
mov w1, w0
adrp x0, .LC1
add x0, x0, :lo12:.LC1
bl printf
I also use "pmccabe" , a tool from gcc, to calculate the complexity of the code.
It shows this patch can reduce the statements in function.
Use if(A) and if(!A):
pmccabe -v test.c Modified McCabe Cyclomatic Complexity
| Traditional McCabe Cyclomatic Complexity
| | # Statements in function
| | | First line of function
| | | | # lines in function
| | | | | filename(definition line number):function
| | | | | |
3 3 8 4 17 test.c(4): main
Use if(A) and else:
pmccabe -v test.c
Modified McCabe Cyclomatic Complexity
| Traditional McCabe Cyclomatic Complexity
| | # Statements in function
| | | First line of function
| | | | # lines in function
| | | | | filename(definition line number):function
| | | | | |
2 2 7 4 16 test.c(4): main
So I think this type of patchs is meaningful.
Thanks,
Wan Jiabing
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] FDDI: defxx: simplify if-if to if-else
2022-04-24 23:26 ` Maciej W. Rozycki
2022-04-25 3:27 ` Jiabing Wan
@ 2022-04-25 12:06 ` Andrew Lunn
1 sibling, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2022-04-25 12:06 UTC (permalink / raw)
To: Maciej W. Rozycki
Cc: Wan Jiabing, David S. Miller, Jakub Kicinski, Paolo Abeni,
netdev, linux-kernel, kael_w
On Mon, Apr 25, 2022 at 12:26:10AM +0100, Maciej W. Rozycki wrote:
> On Mon, 25 Apr 2022, Andrew Lunn wrote:
>
> > > NAK. The first conditional optionally sets `bp->mmio = false', which
> > > changes the value of `dfx_use_mmio' in some configurations:
> > >
> > > #if defined(CONFIG_EISA) || defined(CONFIG_PCI)
> > > #define dfx_use_mmio bp->mmio
> > > #else
> > > #define dfx_use_mmio true
> > > #endif
> >
> > Which is just asking for trouble like this.
> >
> > Could i suggest dfx_use_mmio is changed to DFX_USE_MMIO to give a hint
> > something horrible is going on.
>
> There's legacy behind it, `dfx_use_mmio' used to be a proper variable and
> references were retained not to obfuscate the changes that ultimately led
> to the current arrangement. I guess at this stage it could be changed to
> a function-like macro or a static inline function taking `bp' as the
> argument.
Yes, something like that would be good.
> > It probably won't stop the robots finding this if (x) if (!x), but
> > there is a chance the robot drivers will wonder why it is upper case.
>
> Well, blindly relying on automation is bound to cause trouble.
Unfortunately, there are a number of bot drivers who do blindly rely
on automation. We have had to undo the same broken bot driven changes
a few times, and ended up adding extra comments to catch the eye of
the bot drivers.
Andrew
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-25 12:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-24 9:28 [PATCH] FDDI: defxx: simplify if-if to if-else Wan Jiabing
2022-04-24 10:39 ` Maciej W. Rozycki
2022-04-24 22:17 ` Andrew Lunn
2022-04-24 23:26 ` Maciej W. Rozycki
2022-04-25 3:27 ` Jiabing Wan
2022-04-25 12:06 ` Andrew Lunn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.