The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
as the "else" branch. Delete it to simplify code.
No functional change.
Signed-off-by: Zhen Lei <redacted>
---
drivers/net/ethernet/nvidia/forcedeth.c | 3 ---
1 file changed, 3 deletions(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-05-10 18:31:28
On Mon, 10 May 2021 22:19:34 +0800 Zhu Yanjun wrote:
On Mon, May 10, 2021 at 9:57 PM Zhen Lei [off-list ref] wrote:
quoted
The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
as the "else" branch. Delete it to simplify code.
No functional change.
Thanks.
Missing Fixes?
Fixes tag is supposed to be used for functional fixes.
This patch (and the stmmac one) removes a branch based on the fact that
it's the same as the default / catch all case. It's has a net negative
effect on the reability of the code since now not all cases are
explicitly enumerated. But it's at least the 3rd time we got that
stmmac patch so perhaps not worth fighting the bots...
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-05-10 19:53:17
This patch (and the stmmac one) removes a branch based on the fact that
it's the same as the default / catch all case. It's has a net negative
effect on the reability of the code since now not all cases are
explicitly enumerated. But it's at least the 3rd time we got that
stmmac patch so perhaps not worth fighting the bots...
Hi Jakub
Is it the same bot every time? Or are the masters of the bots learning
what good code actually looks like and fixing their bots? Unless we
push back, the bot masters are not going to get any better at managing
their bots.
Andrew
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-05-10 20:00:12
On Mon, May 10, 2021 at 09:56:56PM +0800, Zhen Lei wrote:
The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
as the "else" branch. Delete it to simplify code.
No functional change.
Signed-off-by: Zhen Lei <redacted>
Hi Zhen
Could you teach your bot to check lore.kernel.org and see if the same
patch has been submitted before? If it has, there is probably a reason
why it was rejected. You need to check if that reason it still true.
Andrew
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-05-10 23:31:50
On Mon, 10 May 2021 21:52:38 +0200 Andrew Lunn wrote:
quoted
This patch (and the stmmac one) removes a branch based on the fact that
it's the same as the default / catch all case. It's has a net negative
effect on the reability of the code since now not all cases are
explicitly enumerated. But it's at least the 3rd time we got that
stmmac patch so perhaps not worth fighting the bots...
Hi Jakub
Is it the same bot every time? Or are the masters of the bots learning
what good code actually looks like and fixing their bots? Unless we
push back, the bot masters are not going to get any better at managing
their bots.
On Mon, May 10, 2021 at 09:56:56PM +0800, Zhen Lei wrote:
quoted
The statement of the last "if (adv_lpa & LPA_10HALF)" branch is the same
as the "else" branch. Delete it to simplify code.
No functional change.
Signed-off-by: Zhen Lei <redacted>
Hi Zhen
Could you teach your bot to check lore.kernel.org and see if the same
patch has been submitted before? If it has, there is probably a reason
why it was rejected. You need to check if that reason it still true.
This is a tool that comes with the kernel. Now it's all about manual
Google searches to see if someone has posted it. So there could be a
mistake.
Although the compiler can optimize this "if" branch, but I think those
that can optimize directly should try to avoid relying on the machine.
If it must exist, it should be in the form of comments. Otherwise, the
intuition is that there was a mistake in writing this code. That's why
the kernel tool reports it. At least the developers of the tool has the
same point of view as mine.
From: Leon Romanovsky <leon@kernel.org> Date: 2021-05-11 05:27:20
On Mon, May 10, 2021 at 04:31:47PM -0700, Jakub Kicinski wrote:
On Mon, 10 May 2021 21:52:38 +0200 Andrew Lunn wrote:
quoted
quoted
This patch (and the stmmac one) removes a branch based on the fact that
it's the same as the default / catch all case. It's has a net negative
effect on the reability of the code since now not all cases are
explicitly enumerated. But it's at least the 3rd time we got that
stmmac patch so perhaps not worth fighting the bots...
Hi Jakub
Is it the same bot every time? Or are the masters of the bots learning
what good code actually looks like and fixing their bots? Unless we
push back, the bot masters are not going to get any better at managing
their bots.
From: Andrew Lunn <andrew@lunn.ch> Date: 2021-05-11 12:20:33
If it must exist, it should be in the form of comments. Otherwise, the
intuition is that there was a mistake in writing this code.
I agree that it could look like there is an error in the code. But
that is where the bot stops and the human takes over. When you read
the code, and understand what it does, you can see there is no
error. You should also look at the history. This is old code, if it
was broken, it would of been fixed by now, since the hulk bot has been
around for quiet a while.
For me, the optimization argument is not correct. We have a lot of
code, in macros for example, where we assume the compiler will
optimize it. Think about all the
if (IS_ENABLED(CONFIG_FOO))
{}
code. The code you are suggesting to change is also on the very slow
path. We want to optimize it for human understandability, not code
generation. It is much more important humans understand it, than the
few microsecods it takes the compiler to optimize it.
Andrew