On Tue, Mar 8, 2022 at 6:12 AM Vincent Mailhol
<[email protected]> wrote:
>
> This patch silences a -Wtypes-limits warning in GENMASK_INPUT_CHECK()
> which is accountable for 31% of all warnings when compiling with W=2.
Please, just make the patch be "remote -Wtypes-limits".
Instead of making an already complicated check more complicated, and
making it more fragile.
I don't see why that int cast on h would be valid, for example. Why
just h? And should you not then check that the cast doesn't actually
change the value?
But the basic issue is that the compiler warns about bad things, and
the problem isn't the code, but the compiler.
Linus
Hi Linus,
On Wed. 9 Mar 2022 at 03:13, Linus Torvalds
<[email protected]> wrote:
> On Tue, Mar 8, 2022 at 6:12 AM Vincent Mailhol
> <[email protected]> wrote:
> >
> > This patch silences a -Wtypes-limits warning in GENMASK_INPUT_CHECK()
> > which is accountable for 31% of all warnings when compiling with W=2.
>
> Please, just make the patch be "remote -Wtypes-limits".
After this patch, the number of remaining -Wtype-limits drops by
99.7% from 164714 to only 431 for an allyesconfig (some of which
could be true positives). So I am inclined to keep
-Wtype-limits at W=2 because it still catches some relevant
issues. Aside from the issue pointed out here, it is not a hindrance.
> Instead of making an already complicated check more complicated, and
> making it more fragile.
ACK, this patch makes it more complicated. About making it more
fragile, lib/test_bits.c is here to catch issues and this patch
passes those tests including the TEST_GENMASK_FAILURES.
> I don't see why that int cast on h would be valid, for example. Why
> just h?
The compiler only complains on ((unsigned int)foo > 0) patterns,
i.e. when h is unsigned and l is zero. The signness of l is not relevant
here.
> And should you not then check that the cast doesn't actually
> change the value?
The loss of precision only occurs on big values
e.g. GENMASK(UINT_MAX + 1, 0).
GENMASK (and GENMASK_ULL) already requires h and l to be between
0 and 31 (or 63). Out of band positive values are caught by
-Wshift-count-overflow (and negative values by
-Wshift-count-negative).
So the use cases in which the int cast would change h value are
already caught elsewhere.
> But the basic issue is that the compiler warns about bad things, and
> the problem isn't the code, but the compiler.
ACK, the code is not broken, the compiler is guilty. I tend to
agree to the rule "if not broken, don’t fix", but I consider this
patch to be *the exception* because of the outstanding level of
noise generated here.
If my message did not convince you, then I am fine to move
-Wtypes-limits from W=2 to W=3 as a compromise. But this is not
my preferred solution because some -Wtypes-limits warnings are
useful.
Yours sincerely,
Vincent Mailhol