On Tue, 2024-08-27 at 15:29 +0800, Hongbo Li wrote: > > On 2024/8/27 15:25, Johannes Berg wrote: > > On Tue, 2024-08-27 at 11:03 +0800, Hongbo Li wrote: > > > The following Coccinelle/coccicheck warning reported by > > > minmax.cocci: > > > WARNING opportunity for max() > > > > Yeah well, maybe sometimes we shouldn't blindly follow tools ... > > > > > Let's use max() to simplify the code and fix the warning. > > > > You should explain why. > > > > I think only one out of four changes in this patch is correct, > > semantically. > > > You mean sometimes we should keep the variable type in comparison? No, I just don't think these are semantically calculations of a maximum, even if they look that way. That's why I asked: Why are you making this change? It looks like you're making this change just because you want coccicheck to be silent here. But that's *really* not a good reason! Don't do that, ever, *think* about the changes you're making too. We should consider the primary consumer of the code to be *people*, not the compiler or tools like coccicheck. And for *people*, applying max() to a link ID makes no sense. It's a link ID, not any kind of value that applying max() to makes any sense. In contrast, for the timeout value there that you changed, that _does_ make sense: it clearly wants to take the longer of the two durations. So then why do we have patterns that look like max(0, link_id)? That's because we treat -1 as a special value indicating "no link, but for the whole sta/vif/...", "don't care about the link" or "MLD not used" (depending on the context). Internally in the code, however, we use 0 for non-MLD to simplify older drivers and internal logic. That's why we end up with "link_id >= 0 ? link_id : 0" in some places. But it's fundamentally not max() even though it looks like it. Replacing it with max() does this a disservice. Now arguably open-coding it often (though three perhaps isn't often, but I'm surprised it's only three times) maybe isn't a great idea either, but then that should be solved differently. So yeah, please think about changes, don't make them blindly. johannes
