On Sun, 8 Dec 2024 at 10:11, Martin Uecker <muecker@xxxxxxx> wrote: > > > > A lot of the 'macro business' for min/max is avoiding unexpected > > conversion of negative values to very large unsigned ones. > > And no, -Wsign-compare is spectacularly useless. > > This is a different topic, but what would be needed here? Dan Carpenter actually wrote up some of the issues in: https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/ but the basic issue is that -Wsign-compare has over the years been truly spectacularly bad. It has literally started out from the completely nonsensical and incorrect assumption that the types of a comparison have to match in signedness, and it shows in the name itself, but it also showed in early implementations. The very first versions of gcc that did -Wsign-compare literally complained about code like sizeof(x) < 5 because obviously one side is an unsigned 'size_t', and the other side is a signed 'int'. So comparing the two is clearly invalid, right? No. It's obviously *not* invalid, and any compiler that complains about different signedness of that compare is just complete useless garbage. It's literally checking two constants against each other, and the result doesn't depend on the signedness or the silent C implicit type conversion. And no, gcc doesn't complain about that particular code any more. *That* particular problem was I think only visible in a gcc pre-release that sadly did actually ship as part of a SUSE release, so we saw it in the wild even if it was never in an official gcc release. I'm pointing out the history because it's relevant due to explaining *why* the whole concept of looking at just the type is so broken, and how the whole background to the warning was broken from the very beginning. The very name of the warning is a sign of the problem. Because gcc still *does* complain about entirely valid code, where "fixing" the warning just means you have to write worse code. I think Dan's example from the link above is a good one: if for (int i = 0; i < sizeof(x); i++) causes a warning, the compiler got things entirely wrong. And yes, modern gcc very much warns about that: t.c:4:27: warning: comparison of integer expressions of different signedness: ‘int’ and ‘long unsigned int’ [-Wsign-compare] 4 | for (int i = 0; i < sizeof(b); i++) | ^ So if you want a general-purpose "Warn about dangerous comparisons", you need to get away from the mindset that it's about different signs. A compiler needs to do proper value range analysis before warning about comparing said values. Not just mindlessly say "different types bad" like some marsupial that has been dropped on its head a few too many times. End result: calling it "Warn about sign compare" is a disease. It shows a lack of understanding of how complex the warning logic needs to be. Now, I'm not claiming that our min/max type warnings are great either: they *do* end up basically being the same silly "just check signs, but at least don't complain about signed positive constants being used for unsigned comparisons". So our min/max macros most definitely are *not* doing that "value range analysis" that I claim is required for a *general* comparison thing. But our min//max macros aren't some general thing. They are very specific, and so it's a lot easier to accept the not-great-analysis for those specific cases where we then may have to change types explicitly or do some other massaging to avoid the warning. Put another way: a warning that triggers on really basic C absolutely *must*not* have silly easily triggerable false positives for good and idiomatic source code. Such a warning is worse than useless, and gets disabled. But a warning that is overly restrictive and gives silly false positives can still be entirely acceptable when the context of that warning is very limited. So this is why in the kernel we disable '-Wsign-compare' in the general case, but *do* basically manually then implement that very same logic in the very _specific_ case of the min/max() macros. What is unacceptable nonsense in one case may be acceptable "good enough" in another. Life is not fair, I'm afraid. Linus
