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

[Ganesh Ajjanagadde]

On Mon, Oct 5, 2015 at 4:09 PM, Ganesh Ajjanagadde
<gajjanagadde at gmail.com> wrote:
> 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.

It maybe helpful if there is a comparison. I have thus created a patch
for avfilter/buffersrc that uses this attribute in a useful, minimal
way.

>
>>
>> Ronald