Re: [PATCH 08/10] seccomp.2: Remove unneeded cast

[Alejandro Colomar <colomar.6.4.3@gmail.com>]

On 2020-09-25 10:34, Michael Kerrisk (man-pages) wrote:
> Hi Alex,
> 
> On 9/25/20 9:31 AM, Alejandro Colomar wrote:
>> Signed-off-by: Alejandro Colomar <colomar.6.4.3@gmail.com>
>> ---
>>   man2/seccomp.2 | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/man2/seccomp.2 b/man2/seccomp.2
>> index 58033da1c..d6b856c32 100644
>> --- a/man2/seccomp.2
>> +++ b/man2/seccomp.2
>> @@ -1101,7 +1101,7 @@ install_filter(int syscall_nr, int t_arch, int f_errno)
>>       };
>>   
>>       struct sock_fprog prog = {
>> -        .len = (unsigned short) (sizeof(filter) / sizeof(filter[0])),
>> +        .len = sizeof(filter) / sizeof(filter[0]),
>>           .filter = filter,
>>       };
> 
> I have a small doubt about this change. With the change,
> there are no compilation warnings.
> 
> But, if we change the code to something slightly different:
> 
> [[
>      size_t x = (sizeof(filter) / sizeof(filter[0]));
>      struct sock_fprog prog = {
>          .len = x,
>          .filter = filter,
>      };
> ]]
> 
> The "cc -Wconversion" gives us the following warning:
> 
>      warning: conversion from ‘size_t’ {aka ‘long unsigned int’}
>      to ‘short unsigned int’ may change value
> 
> Presumably we don't get a warning for an assignment of the form
> 
>      .len = (sizeof(filter) / sizeof(filter[0]))
> 
> because the compiler is smart enough to work out that the
> value of the constant expression is within the range of
> "unsigned short".
> 
> Your thoughts?

Hi Michael,

I'd say that the cast doesn't fix any problems at all.  It silences a 
valid warning, and I'd use a pragma for that (to be more explicit about 
the intention of silencing a warning) if I do want -Wconversion enabled 
(which usually I don't want, because it's too noisy) and I'm sure that 
this won't overflow.  I'd limit the use casts to only when I *really* 
need to.

I guess that if you enable -O3, the warning will vanish again because 
the compiler will optimize away 'x' (but I didn't test).

Thanks,

Alex

> 
> Thanks,
> 
> Michael
> 
>