[FFmpeg-devel] [PATCH 2/2] avfilter/formats: add av_warn_unused_result to function prototypes

Ganesh Ajjanagadde gajjanagadde at gmail.com
Mon Oct 5 22:00:05 CEST 2015


On Mon, Oct 5, 2015 at 3:45 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Mon, Oct 5, 2015 at 3:20 PM, Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> wrote:
>>
>> This uses the av_warn_unused_result attribute liberally to catch some
>> forms of improper
>> usage of functions defined in avfilter/formats.h.
>>
>> Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> ---
>>  libavfilter/formats.h | 38 +++++++++++++++++++-------------------
>>  1 file changed, 19 insertions(+), 19 deletions(-)
>>
>> diff --git a/libavfilter/formats.h b/libavfilter/formats.h
>> index 5a8ee5e..e01be4a 100644
>> --- a/libavfilter/formats.h
>> +++ b/libavfilter/formats.h
>> @@ -116,25 +116,25 @@ typedef struct AVFilterChannelLayouts {
>>   * If a and b do not share any common elements, neither is modified, and
>> NULL
>>   * is returned.
>>   */
>> -AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts
>> *a,
>> +av_warn_unused_result AVFilterChannelLayouts
>> *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>>                                                   AVFilterChannelLayouts
>> *b);
>> -AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a,
>> +av_warn_unused_result AVFilterFormats
>> *ff_merge_samplerates(AVFilterFormats *a,
>>                                        AVFilterFormats *b);
>
>
> I'm not sure this is the intention of warn_unused_result. My understanding
> is that you use this for strict error reporting only, i.e. "you need to
> check this return code for errors because it may fail!", not for "if you
> don't store the result of this call, you're not using my API correctly".
>
> The thing is, if I use ff_merge_samplerates() and I don't store the result,
> my application cannot possibly function correctly. It's not possible for it
> to function correctly. Imagine the following application:
>
> int main() {
>   malloc(2);
>   return 0;
> }
>
> My application cannot possibly function as intended without storing the
> result of malloc(). Yet I don't think anyone is seriously considering
> marking malloc with warn_unused_result. Likewise, we should only use
> warn_unused_result for cases where your application probably works correctly
> without checking a return value, but you should check the return value
> anyway because in real usage, the value may indicate problems that the
> application should be aware of before continuing its operations, e.g.
> avformat_open_input() or something like that.

Personally, I don't mind either way - I do know the original intent
which is along the lines of what you wrote here, but I see no harm in
extending to things like malloc, or in our case av_malloc. Of course
the utility is greatly reduced, since as you point out essentially
anyone who writes that kind of code (malloc(2), return 0) has no
business writing C in general. Nevertheless, I don't understand the
concrete harm - they are never false positives with things like
malloc, and the only negative side effect is the more verbose
declaration.

In fact, it is this verbosity that makes me ambivalent between
applying this liberally versus the standard, more cautious approach -
otherwise I am all in favor of placing this pretty much all over.

>
> Ronald


More information about the ffmpeg-devel mailing list