Обсуждение: hba.c:3160:18: warning: comparison of unsigned enum expression
Recently (last day or so), I get this warning from gcc 10.2:
-----
hba.c:3160:18: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
if (auth_method < 0 || USER_AUTH_LAST < auth_method)
~~~~~~~~~~~ ^ ~
1 warning generated.
-----
Erik
On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote:
>
> Recently (last day or so), I get this warning from gcc 10.2:
>
> -----
> hba.c:3160:18: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare]
> if (auth_method < 0 || USER_AUTH_LAST < auth_method)
> ~~~~~~~~~~~ ^ ~
> 1 warning generated.
> -----
This one is from 9afffcb833d3c5e59a328a2af674fac7e7334fc1 (adding
Jacob and Michael to cc)
And it makes sense to give warning on that. AuthMethod is an enum. It
can by definition not have a value that's not in the enum. That check
simply seems wrong/unnecessary.
The only other use fo USER_AUTH_LAST is in fill_hba_line() which also
gets the name of the auth. That one uses :
StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
"UserAuthName[] must match the UserAuth enum");
Which seems like a more reasonable check.
But that also highlights -- shouldn't that function then also be made
to use hba_authname(), and the assert moved into the function? That
seems like the cleanest way?
--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/
On Wed, Apr 7, 2021 at 1:24 PM Magnus Hagander <magnus@hagander.net> wrote: > > On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote: > > > > Recently (last day or so), I get this warning from gcc 10.2: > > > > ----- > > hba.c:3160:18: warning: comparison of unsigned enum expression < 0 is always false [-Wtautological-compare] > > if (auth_method < 0 || USER_AUTH_LAST < auth_method) > > ~~~~~~~~~~~ ^ ~ > > 1 warning generated. > > ----- > > This one is from 9afffcb833d3c5e59a328a2af674fac7e7334fc1 (adding > Jacob and Michael to cc) > > And it makes sense to give warning on that. AuthMethod is an enum. It > can by definition not have a value that's not in the enum. That check > simply seems wrong/unnecessary. > > The only other use fo USER_AUTH_LAST is in fill_hba_line() which also > gets the name of the auth. That one uses : > StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1, > "UserAuthName[] must match the UserAuth enum"); > > Which seems like a more reasonable check. > > But that also highlights -- shouldn't that function then also be made > to use hba_authname(), and the assert moved into the function? That > seems like the cleanest way? So to be clear, this is what I'm suggesting. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Вложения
On Wed, Apr 07, 2021 at 01:24:01PM +0200, Magnus Hagander wrote: > On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote: > > Recently (last day or so), I get this warning from gcc 10.2: Same compiler version here, but I did not get warned. Are you using any particular flag? > But that also highlights -- shouldn't that function then also be made > to use hba_authname(), and the assert moved into the function? That > seems like the cleanest way? Good idea, that's much cleaner this way. Do you like the attached? -- Michael
Вложения
On Wed, Apr 7, 2021 at 1:57 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Apr 07, 2021 at 01:24:01PM +0200, Magnus Hagander wrote: > > On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote: > > > Recently (last day or so), I get this warning from gcc 10.2: > > Same compiler version here, but I did not get warned. Are you using > any particular flag? > > > But that also highlights -- shouldn't that function then also be made > > to use hba_authname(), and the assert moved into the function? That > > seems like the cleanest way? > > Good idea, that's much cleaner this way. Do you like the attached? That's very close to mine (see one email later). Let's bikeshed about the details. I think it's basically the same for current usecases, but that taking the UserAuth as the parameter is cleaner and potentially more useful for the future. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
On Wed, Apr 07, 2021 at 02:01:42PM +0200, Magnus Hagander wrote: > That's very close to mine (see one email later). Let's bikeshed about > the details. I think it's basically the same for current usecases, but > that taking the UserAuth as the parameter is cleaner and potentially > more useful for the future. Missed it, sorry about that. Using UserAuth as argument is fine by me. If you wish to apply that, please feel free. I am fine to do that myself, but that will have to wait until tomorrow my time. -- Michael
Вложения
> On 2021.04.07. 13:57 Michael Paquier <michael@paquier.xyz> wrote: > > > On Wed, Apr 07, 2021 at 01:24:01PM +0200, Magnus Hagander wrote: > > On Wed, Apr 7, 2021 at 1:01 PM Erik Rijkers <er@xs4all.nl> wrote: > > > Recently (last day or so), I get this warning from gcc 10.2: > [gcc-hba-warning.patch] FWIW, this fixes the warning. (and no, I don't think I am using special gcc settings..) Erik
On Wed, Apr 7, 2021 at 2:17 PM Michael Paquier <michael@paquier.xyz> wrote: > > On Wed, Apr 07, 2021 at 02:01:42PM +0200, Magnus Hagander wrote: > > That's very close to mine (see one email later). Let's bikeshed about > > the details. I think it's basically the same for current usecases, but > > that taking the UserAuth as the parameter is cleaner and potentially > > more useful for the future. > > Missed it, sorry about that. Using UserAuth as argument is fine by > me. If you wish to apply that, please feel free. I am fine to do > that myself, but that will have to wait until tomorrow my time. Ok, I'll go ahead and push it. Thanks for confirming the fix! -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
On Wed, Apr 07, 2021 at 02:20:25PM +0200, Magnus Hagander wrote: > Ok, I'll go ahead and push it. Thanks for confirming the fix! Cool. Thanks! -- Michael