Messages in this thread | | | From | Vincent MAILHOL <> | Date | Tue, 8 Mar 2022 21:20:32 +0900 | Subject | Re: [PATCH] linux/bits.h: fix -Wtype-limits warnings in GENMASK_INPUT_CHECK() |
| |
On Tue. 8 Mar 2022 à 01:30, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Mon, Mar 7, 2022 at 5:10 PM Alexander Lobakin > <alexandr.lobakin@intel.com> wrote: > > From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr> > > Date: Mon, 7 Mar 2022 22:50:56 +0900 > > ... > > > For example, people tend to make the following mistake: > > > > unsigned int i; > > > > for (i = 0; i ...) { > > ret = setup_something(array[i]); > > if (ret) > > goto unroll; > > } > > > > unroll: > > while (--i) > > unroll_something(array[i]); > > > > The loop will never end as `i` was declared as unsigned. > > -Wtype-limits catches this. > > This looks like a wrapping value issue, not sure if the type limits > makes logical sense. What I'm saying is that the waning is > controversial. It may help or it may make noise. > > > Not speaking of checking unsigned variables on < 0: > > > > unsigned int num; > > > > /* calculate_something() returns the number of something > > * or -ERRNO in case of an error > > */ > > num = calculate_something(); > > if (num < 0) > > ... > > Depends on the context. Here is a mistake, but there are plenty of > cases when it's okay to do so.
I am curious to see which case you are thinking of.
Personally, I see two cases, both with macro:
1/ Cases similar to GENMASK_INPUT_CHECK() in which the macro is made for generic purpose and in which there was no way to know in advance that one parameter would be unsigned and the other zero.
2/ Comparaison again a macro which might or might not be zero. e.g.:
#ifdef FOO #define BAR 42 #else #define BAR 0 #endif
void baz(void) { unsigned int i;
if (i > BAR) /* yields -Wtype-limits if FOO is not defined. */ }
And because of those two false positives, moving it to W=2 was a wise choice.
But I am not aware of any use cases outside of macro where doing an:
unsigned int num; /* ... */ if (num < 0)
would be okay. At best it is dead code, at worse, it is a bug as pointed out by Alexander.
I am not sure what I am missing here.
> And in the above the variable name is > misleading with its semantics, The proper code should be > > unsigned int num; > int ret; > > ret = ... > if (ret < 0) > ... > num = ret; > > Again, the warning is controversial in my opinion. > > -- > With Best Regards, > Andy Shevchenko
| |