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

[Ronald S. Bultje]

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.

(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.)

Ronald