[vlc-devel] [PATCH v4 3/5] display: add macro to set the callbacks and check type of the Open/Close

[Steve Lhomme]

On 2019-07-10 13:51, Rémi Denis-Courmont wrote:
> Hi,
> 
> A static_assert failure *is* an error. 

"more than an error" is an error.

Mismatched function pointer types should be a warning, not an error.
> 

I strongly disagree. This is a security hazard left unattended.

> You're free to use Werror on your builds if you want.
> 
> Le 10 juillet 2019 14:35:17 GMT+03:00, Steve Lhomme <robux4 at ycbcr.xyz> a écrit :
>> On 2019-07-10 12:42, Rémi Denis-Courmont wrote:
>>> Hi,
>>>
>>> As already stated yesterday, I don't see why this specific case
>> grants an error any more than any other function pointer passing.
>>
>> It's more than an error, it's a static assert. It won't compile if you
>> use the wrong signature. IMO it's a good step in better code safety.
>>
>> One drawback of this approach is when you change a signature you have
>> to
>> change it in all modules, whereas before you could change the signature
>>
>> and then each module individually. Not breaking compilation, just the
>> code running temporarily.
>>
>> Warnings are usually ignored (see my other patches fixing long time
>> warnings).
>>
>> We could have a static_assert in debug build and a warning in release
>> builds.
>>
>>> In any case, this wrongly causes an error if zero is passed as
>> deactivate callback or if NULL is an integer.
>>
>> And if NULL is passed instead of nullptr in C++. It's easy enough to
>> use
>> the proper value in that case.
>>
>> For NULL I wasn't sure if it's always a void* in C11. It is in Clang
>> (linux, win32), gcc (win32) and MSVC.
>>
>> There's typeof(NULL) but it seems to be a GNU extension :
>> https://stackoverflow.com/questions/40143815/is-it-possible-to-implement-gnu-cs-typeofx-with-c11s-generic
>>
>>> Le 10 juillet 2019 11:39:06 GMT+03:00, Steve Lhomme
>> <robux4 at ycbcr.xyz> a écrit :
>>>> ---
>>>> include/vlc_vout_display.h | 17 +++++++++++++++++
>>>> 1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/include/vlc_vout_display.h b/include/vlc_vout_display.h
>>>> index fd4940cf6d..1cbd6b1891 100644
>>>> --- a/include/vlc_vout_display.h
>>>> +++ b/include/vlc_vout_display.h
>>>> @@ -215,6 +215,23 @@ typedef int
>> (*vout_display_open_cb)(vout_display_t
>>>> *vd,
>>>>    */
>>>> typedef void (*vout_display_close_cb)(vout_display_t *vd);
>>>>
>>>> +#ifndef __cplusplus
>>>> +#define set_callbacks_display(Activate, Deactivate) \
>>>> +    static_assert( _Generic( Activate, vout_display_open_cb: 1,
>>>> default: 0 ) == 1, \
>>>> +                  "Not a display activate callback"); \
>>>> +    static_assert( _Generic( Deactivate, vout_display_close_cb: 1,
>>>> void*: 1, default: 0 ) == 1, \
>>>> +                  "Not a display deactivate callback"); \
>>>> +    set_callbacks( Activate, Deactivate )
>>>> +#else /* __cplusplus */
>>>> +#define set_callbacks_display(Activate, Deactivate) \
>>>> +    static_assert(
>>>> std::is_same<std::add_pointer<decltype(Activate)>::type,
>>>> vout_display_open_cb>::value, \
>>>> +                  "Not a display activate callback"); \
>>>> +    static_assert(
>>>> std::is_same<std::add_pointer<decltype(Deactivate)>::type,
>>>> vout_display_close_cb>::value || \
>>>> +
>>>> std::is_same<decltype(Deactivate),std::nullptr_t>::value, \
>>>> +                  "Not a display deactivate callback"); \
>>>> +    set_callbacks( Activate, Deactivate )
>>>> +#endif /* __cplusplus */
>>>> +
>>>> struct vout_display_t {
>>>>       struct vlc_object_t obj;
>>>>
>>>> -- 
>>>> 2.17.1
>>>>
>>>> _______________________________________________
>>>> vlc-devel mailing list
>>>> To unsubscribe or modify your subscription options:
>>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>
>>> -- 
>>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>> excuser ma brièveté.
>>>
>>>
>>> _______________________________________________
>>> vlc-devel mailing list
>>> To unsubscribe or modify your subscription options:
>>> https://mailman.videolan.org/listinfo/vlc-devel
>>>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
> 
> -- 
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
> 
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
>