[vlc-devel] commit: signals: Return NULL at SigThread end. (Pierre d'Herbemont )

[Pierre d'Herbemont]

On Aug 21, 2009, at 5:51 PM, Rémi Denis-Courmont wrote:

> Le vendredi 21 août 2009 11:40:16 Pierre d'Herbemont, vous avez  
> écrit :
>>>> The above solution is the most portable one, I think.
>>>
>>> No. Some compilers generate warning on dead code, and some compilers
>>> generate
>>> warnings in both cases. There are no warning-free "portable"
>>> solutions. The
>>> only portable solution is to not turn warnings into errors. In fact,
>>> turning
>>> warnings into errors is not portable. Suck it up.
>>
>> Yes. That's why we are only turning on to error specific class of
>> warnings, not all warnings :-)
>
> You're missing the point. Occurences of warnings depend on the  
> compiler,
> compiler version, architecture, targetted run-time, optimization  
> level (at
> least). There is no portable definition of avoidable warnings. For  
> instance,
> if you enable errors on unused results, OS X will probably build  
> fine, but
> Linux _will_ fail. If you enable errors on incompatible pointer  
> types, Windows
> _will_ fail but probably not Linux. If you enable alignment  
> upcasting errors,
> ARM will fail, but x86 will do fine.
>
> This particular case is a good example. GCC itself will generate  
> different
> warnings depending on its version and optimization level. If it  
> detects that
> this is dead code, it will complain about dead code. Then again, it  
> might -
> wrongly- complain about lack of return value.

I see your point better, however, I still consider this to be  
anecdotical or less important than the warnings sanitization.

Also, as time advance, we'll get a better platform coverage and tuning  
with --enable-treat-warnings-as-error, this way.

This might seems like a dictatorial solution but I believe this will  
actually help VLC development.

I believe that if it's good to make sure that contributors and build  
bot build VLC with --enable-treat-warnings-as-error. We'll filter out  
warnings. It seems to me that this solution is the most realistic to  
force adoption. We let packagers use --disable-treat-warnings-as-error.

I can conceive that we could do the other way around, and if there is  
too much complaint we could --enable-treat-warning-as-error on build  
bot and find a way to get contributor to build with it. Yet, I fear  
that this could mean simply ignoring deadly warnings again.

So if It doesn't work on your specific arch platform combo:
- You don't feel like fixing it, fine add --disable-treat-warnings-as- 
error.
or
- You like fixing warnings and issue like this, then add a local  
exception for your configuration.

So far, among the other contributors, this change was well perceived.  
And their builds seems to be OK.

>>>
>>> This is a stupid idea, even in debug mode. If you want to enable for
>>> yourself,
>>> it's your problem. On ARM Linux, I have unfixable warnings due to
>>> alignment vs
>>> cast problems. With threads cancellation, I also get dummy clobber
>>> warnings:
>>> the only solution would be to use "volatile" which is just worse
>>> than getting
>>> a warning, IMHO. We also have warnings due to unused results from
>>> glibc, in
>>> some cases where we actually know what we are doing.
>>
>> That's why we don't enable warning on error for such warnings.
>
> And how do you know that it's fine to enable them in ANY case?? Only  
> those
> warnings that are *always* avoidable and very likely actual errors are
> acceptable. Sorry but unused parameters or unused variables simply  
> do *not*
> fit that definition. Indeed, hardly any of the recent "warnings fix"  
> addressed
> any actual *run-time* bug.


My take on this: When a warning is reviewed by someone as being  
benign, it should be avoided, so that we don't get false warnings.  
That the story of the kid that falsely scream that there is wolf... :)

I am also not too keen (after all) of #warning to indicate FIXME or  
todo. This should probably in code, in comment and probably as a trac  
ticket.

Pierre.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20090821/1e8dc98f/attachment.html>