On Thu, May 04, 2023 at 05:58:17PM +0800, Dongliang Mu wrote: > Hi Dan, > > I have some questions about one checker result [1]: > > drivers/scsi/huawei/hifc/unf_exchg.c:484 unf_get_xchg_config_sum() > warn: replace divide condition '*v_xchg_sum / (4)' with '*v_xchg_sum > >= (4)' > > 1. What's the rule of this checker? > If the code compares the quotient with zero, let's change it to a > compare operation? This is kind of optimization? This rule was motivated by a bug that looked like: x = foo / bar ? a : b; The code should have been something completely unrelated like checking for negative or something. I forget. But it just seemed like a nonsense thing to check a divide like that. It's more readable to check if (foo >= bar) and it will execute faster as well. In the kernel, when I ran this check I found some places where people wrote if (foo / bar) but they had intended to write if (foo % bar). But then it turned out that where were a lot of places where it was more readable to always write "foo / bar". x = foo / bar; if (foo / bar) frob(foo / bar); So this check has some false positives. I might delete it. Looking at that code, the error message prints "foo less than bar", so I really feel like checking for foo < bar is more readable. But that's a style debate. I might end up just deleting this check instead. > 2. Why does the suggestion change it to ">="? > >From my understanding, it should use "<". Correct. The Smatch check just gets the divide part of the condition and not the == 0 part. > > [1] https://gitee.com/openeuler/kernel/blob/openEuler-22.03-LTS-SP1/drivers/scsi/huawei/hifc/unf_exchg.c#L484 regards, dan carpenter
