[FFmpeg-devel] [PATCH] incorrect return type for opt_bitrate/type safety

[Morten Hustveit]

On Wed, Jan 02, 2008 at 12:46:38AM +0100, Michael Niedermayer wrote:
> adding const to stuff should be a seperate patch
[...]
> fixing the return type should also be a seperate patch
[...]
> this is ugly, either way, it should be a sperate patch
[...]
> changing (void*) to the nicer stuff is a cosmetic change and MUST be a 
> seperate patch

I do not agree with your patch granularity preferences.  The largest patch
depends on each of the small patches, but was written first.  The small patches
are mistakes that were discovered by the "cosmetic change" (excluding
opt_bitrate, which was discovered in advance).

Either way, I have split the patch in the suggested fashion.

> > -static uint64_t limit_filesize = 0; //
> > +static int64_t limit_filesize = 0; //
> 
> filesize is not signed

Signed is Correct:

  - The variable is used only in signed contexts.
  - ffmpeg's offset_t is signed.
  - In POSIX, off_t is used for file sizes, and off_t is signed. [1]

Signed is Sufficient[tm]:

  If you are going to limit the file size, it will be to less
  than 8.5 billion gigabytes in at least 9 cases out of 10.
 
>> -static void opt_new_audio_stream(void)
>> +static void opt_new_audio_stream(const char *arg av_unused)

> this is ugly, [...]

You have four choices:

  1. Use the current incorrect function signature and hide the fact by emulating
     implicit casts with an explicit void* cast.
  2. Make the current function signature correct by adding yet another option
     type to optutils.h.
  3. Use the correct function signature and disregard the parameter.
  4. Ignore compiler warnings.

I find #1, #2 and #4 uglier than #3.  #1 and #4 makes bugs harder to spot
(opt_bitrate is an example of such bug).  #2 increases the number of code lines
without adding functionality or fixing bugs.

1. http://www.opengroup.org/onlinepubs/007908775/xsh/systypes.h.html

-- 
Morten Hustveit
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 01-const_correctness.patch
Type: text/x-diff
Size: 908 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080102/b5aa9047/attachment.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 02-sign_correctness.patch
Type: text/x-diff
Size: 411 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080102/b5aa9047/attachment-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 03-signature_correctness.patch
Type: text/x-diff
Size: 1747 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080102/b5aa9047/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 04-return_value_correctness.patch
Type: text/x-diff
Size: 1066 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080102/b5aa9047/attachment-0003.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 05-fake_implicit_cast_removal.patch
Type: text/x-diff
Size: 23244 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080102/b5aa9047/attachment-0004.patch>