[vlc-devel] [PATCH 1/2] Fix unused variable compilation warnings

[Rémi Denis-Courmont]

Le 2014-12-08 15:16, Eduardo Vieira a écrit :
> Hi,
>
> Thanks for the comments.
>
> 2014-12-08 7:30 GMT-02:00 Rémi Denis-Courmont <remi at remlab.net>:
>> Le 2014-12-08 04:06, Eduardo Vieira a écrit :
>>>
>>> ---
>>>  modules/access/zip/zipaccess.c | 1 +
>>>  modules/access/zip/zipstream.c | 1 +
>>>  modules/lua/demux.c            | 1 +
>>>  modules/packetizer/h264.c      | 1 -
>>>  src/input/es_out.c             | 3 +++
>>>  src/input/es_out.h             | 9 +++++++++
>>>  src/misc/picture.c             | 1 +
>>>  src/video_output/display.c     | 1 +
>>>  8 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/modules/access/zip/zipaccess.c
>>> b/modules/access/zip/zipaccess.c
>>> index 2f2b4dc..24064a0 100644
>>> --- a/modules/access/zip/zipaccess.c
>>> +++ b/modules/access/zip/zipaccess.c
>>> @@ -414,6 +414,7 @@ static uLong ZCALLBACK ZipIO_Write( void* 
>>> opaque,
>>> void* stream,
>>>      (void)opaque; (void)stream; (void)buf; (void)size;
>>>      int zip_access_cannot_write_this_should_not_happen = 0;
>>>      assert(zip_access_cannot_write_this_should_not_happen);
>>> +    VLC_UNUSED( zip_access_cannot_write_this_should_not_happen );
>>>      return 0;
>>
>>
>> The defacto VLC style in such case is "assert(0)", which would solve 
>> the
>> problem nicely. However, I suspect this code was meant to stay as 
>> its
>> upstream to make future backport easier...
>
> I looked it up, and it seems the code is this way from the first 
> commit.
> Can I keep the VLC_UNUSED call ?

Either the code must be kept identical and the answer is No.
Or the code need not be kept identical and assert(0) is better.
Or the code is to be removed in favor of libarchive and the question is 
moot.
>>> diff --git a/modules/lua/demux.c b/modules/lua/demux.c
>>> index eac4f24..f9c167b 100644
>>> --- a/modules/lua/demux.c
>>> +++ b/modules/lua/demux.c
>>> @@ -75,6 +75,7 @@ static int vlclua_demux_read( lua_State *L )
>>>          lua_pushlstring( L, (const char *)p_read, i_read );
>>>          int i_seek = stream_Read( p_demux->s, NULL, i_read );
>>>          assert( i_read == i_seek );
>>> +        VLC_UNUSED( i_seek );
>>
>>
>> This assertion is wrong. stream_Read() can fail at any time. So this 
>> patch
>> is wrong.
>
> The patch is not showing, but there's a call to stream_Peek right
> above (in which i_read is set), and the return value is checked to be
>> 0. Do you know if Read can fail even in this case ?

Yes it can.

>>> diff --git a/modules/packetizer/h264.c b/modules/packetizer/h264.c
>>> index c029171..76f7d76 100644
>>> --- a/modules/packetizer/h264.c
>>> +++ b/modules/packetizer/h264.c
>>> @@ -556,7 +556,6 @@ static block_t *CreateAnnexbNAL( decoder_t
>>> *p_dec, const uint8_t *p, int i_size
>>>  static void CreateDecodedNAL( uint8_t **pp_ret, int *pi_ret,
>>>                                const uint8_t *src, int i_src )
>>>  {
>>> -    const uint8_t *end = &src[i_src];
>>
>>
>> That seems like a slightly different problem than the rest of the 
>> patch.
>
> Indeed. In this case, the variable was actually not used, and was not
> within any ifdef or commented code block, so I thought it would be
> safe to erase it.

It's safe to erase but the erasure does not belong in this patch.

>>>      uint8_t *dst = malloc( i_src );
>>>
>>>      *pp_ret = dst;
>>> diff --git a/src/input/es_out.c b/src/input/es_out.c
>>> index 29fee99..ceada38 100644
>>> --- a/src/input/es_out.c
>>> +++ b/src/input/es_out.c
>>> @@ -2565,6 +2565,7 @@ static int EsOutControlLocked( es_out_t *out,
>>> int i_query, va_list args )
>>>          const mtime_t i_date = (mtime_t) va_arg( args, mtime_t );
>>>
>>>          assert( !b_source_paused == !b_paused );
>>> +        VLC_UNUSED( b_source_paused );
>>
>>
>> I'm not sure we can rely on void-casting variables to mute warnings. 
>> The
>> variable is still pointless and some compilers or compiler versions 
>> may
>> still be unhappy either way. That's why this issue and the ones 
>> below were
>> never fixed.
>
> There is an alternative solution by marking the variables using the
> __attribute__((unused)). But I think it's GCC specific, so it will 
> not
> solve the problems you said.

My point is that the compiler may complain that a computed or assigned 
value is never used, instead of complaining that the variable is never 
used. This is no better.

The only clean way to fix this is to remove the variable. And that 
implies either removing the assertion or adding ugly and poorly tested 
#ifdef's. Overall I think we are better off leaving the code as it 
currently is and telling developers to use debug builds for their 
warning hunts.

-- 
Rémi Denis-Courmont