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

[Ganesh Ajjanagadde]

On Mon, Oct 5, 2015 at 4:04 PM, Ronald S. Bultje <rsbultje at gmail.com> wrote:
> Hi,
>
> On Mon, Oct 5, 2015 at 4:00 PM, Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> wrote:
>>
>> 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.
>
>
> I think we typically prefer to apply a policy/approach where one of the
> goals is to write code (including interfaces in header files) that are as
> small as possible. If we litter all our header files with all kind of
> extraneous stuff that isn't really doing anything, they become somewhat
> unreadable.

Thanks for clarifying this.

>
> (I obviously am not claiming that adding one word to one line in one header
> file is the difference between highly readable and unreadable, but it
> suggests a general approach of trending towards smaller source code can be
> one helpful argument in coming up with a proper decision here.)

I can create a patch with the "minimal" set of attributes, namely the
ones I found helpful for the libavfilter cleanup. I will adopt this
approach and post a patch after leaving some more time for discussion.

>
> Ronald