flac dev - Apr 2014 - Patch to fix compiler warnings and error status collisions

On Apr 7, 2014, at 4:07 AM, Erik de Castro Lopo <mle+la at mega-nerd.com>
wrote:
> Lenny Maiorani wrote:
> 
>> I am new around here, so I am not sure of all your procedures
>> for submitting patches/pull requests.
> 
> A patch like you sent is fine. When I commit that I will add
> a Patch-from line to the commit.
> 
> Otherwise, you can commit to you local repo and then use the git
> format-patch sub command to create a set or patches that can
> be attached to an email and sent to the list.
> 
>> The attached patch fixes all Clang compilation warnings.
> 
> I'll check this out.
> 
>> Note, some of these warnings were real problems. There is the
>> potential for API users to misinterpret the state being returned
>> since the return variable was of type FLAC__StreamDecoderState,
>> but the value being returned was FLAC__StreamDecoderInitStatus.
>> When FLAC__STREAM_DECODER_INIT_STATUS_ALREADY_INITIALIZED was
>> returned in this case it was indistinguishable from
>> FLAC__STREAM_DECODER_OGG_ERROR.
> 
> Problem is this patch changes the ABI because FLAC__StreamDecodeInitStatus
> disappears from the API.
> 
> Is there some way this can be fixed without chaning the API/ABI?
FLAC__StreamDecodeInitStatus is still there. It is simply another name for
FLAC__StreamDecoderState. The API does not change here, other than the numerical
value of the enums. However, the bug that exists is that the numerical value of
the enums collide and are being used interchangeably in
decoder->protected_->state, so they really should be the same enum, IMHO.

-Lenny

> 
> Cheers,
> Erik
> -- 
> ----------------------------------------------------------------------
> Erik de Castro Lopo
> http://www.mega-nerd.com/
> _______________________________________________
> flac-dev mailing list
> flac-dev at xiph.org
> http://lists.xiph.org/mailman/listinfo/flac-dev

Lenny Maiorani wrote:
> FLAC__StreamDecodeInitStatus is still there. It is simply another name
> for FLAC__StreamDecoderState. The API does not change here, other than
> the numerical value of the enums.
That is sufficient for an ABI change.
> However, the bug that exists is that the numerical value of the enums
> collide and are being used interchangeably in
decoder->protected_->state,
> so they really should be the same enum, IMHO.
This could also be fixed by adding a:

   decoder->protected_->initstate

and using that correctly. Right?

I'm going to play with this idea.
 
Cheers,
Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/

Erik de Castro Lopo wrote:
> This could also be fixed by adding a:
> 
>    decoder->protected_->initstate
> 
> and using that correctly. Right?
> 
> I'm going to play with this idea.
That fixed it. Commit is here:

   
https://git.xiph.org/?p=flac.git;a=commit;h=3f5208c30022b7cbd0b9095ad3550c4f6cb348c9


What didn't get added was this:

    diff --git a/src/flac/utils.c b/src/flac/utils.c
    index 4bf05e2..941a958 100644
    --- a/src/flac/utils.c
    +++ b/src/flac/utils.c
    @@ -35,7 +35,7 @@
     #ifdef HAVE_TERMIOS_H
     # include <termios.h>
     #endif
    -#ifdef GWINSZ_IN_SYS_IOCTL
    +#if !defined __ANDROID__
     # include <sys/ioctl.h>
     #endif
     #endif

because that seemed completely un-related. What's this bit all about?

Erik
-- 
----------------------------------------------------------------------
Erik de Castro Lopo
http://www.mega-nerd.com/