[Flac-dev] Changes to include/FLAC/metadata.h

[Erik de Castro Lopo]

Josh,

I noticed that the prototypes of two functions in the above header
file recently changed from:

    FLAC_API FLAC__bool
    FLAC__metadata_object_picture_set_mime_type (FLAC__StreamMetadata *object,
          const char *mime_type, FLAC__bool copy);

   FLAC_API FLAC__bool
   FLAC__metadata_object_picture_set_description(FLAC__StreamMetadata *object,
          const FLAC__byte *description, FLAC__bool copy);

to:

    FLAC_API FLAC__bool
    FLAC__metadata_object_picture_set_mime_type (FLAC__StreamMetadata *object,
          char *mime_type, FLAC__bool copy);

   FLAC_API FLAC__bool
   FLAC__metadata_object_picture_set_description(FLAC__StreamMetadata *object,
          FLAC__byte *description, FLAC__bool copy);

In particular, that the mime_type and description parameters changed from
"const char *" and "const FLAC__byte*" pointers to "char *" and "FLAC__byte*"
pointers, dropping the "const" specifier.

Sorry, but I think this is huge mistake. Please let me explain.

The problem is that people may very legitimately want to do things like:

   FLAC__metadata_object_picture_set_mime_type(obj, "image/png", /*copy=*/true);

Now the C compiler will, by default, treat the "image/png" string as read 
only and make the pointer to it a "const char *" pointer. Making the 
function prototype use a non-const pointer, will mean that people like
me who like to compile with a huge number of warning flags enabled, will 
immediately get compiler warnings for the code above where no compiler 
warnings existed before.

I have actually looked at the underlying implementation of these two 
functions and can see that you are caught between a rock and a hard 
place :-). To fix the compiler warnings in the internal FLAC code you 
had to make the pointers non-const. Unfortunately, that solution is
actually worse than the original problem for the reasons I noted above.

The only real solution to this problem is an API change. However, 
changing the pointer from const to non-const is also an API change,
just a more subtle one.

Since it looks like you want to fix this API issue, would you also 
be interested in starting to use other GCC warning flags and then
undertake a more widespread effort to fix all warnings in the 
internal FLAC code?

Enabling more warnings now will have the following benefits:

  - Allow you to ensure that the API will not cause warnings in
    client code even if that client code uses every warning flag
    in existence.

  - Make the FLAC code more portable. Different compilers have 
    different ideas about what is a warning and what is a error.
    Warnings on some C compilers may be errors on others.

  - Enforce const-correct-ness (ie remove all casts from const 
    pointers to non-const pointers).

I myself went throught this effort with libsndfile about five years
ago. Since then I have added functionality to the libsndfile API,
but I have not once broken the ABI.

Since I have some experience in this matter I renew my offer to help.

Cheers,
Erik
-- 
-----------------------------------------------------------------
Erik de Castro Lopo
-----------------------------------------------------------------
"Anyone who considers arithmetical methods of producing random
digits is, of course, in a state of sin." - John Von Neumann (1951)