On date Saturday 2011-05-28 07:32:47 +0200, Anton Khirnov encoded: > > On Fri, 27 May 2011 21:25:21 -0400, "Ronald S. Bultje" <[email protected]> > wrote: > > Hi, > > > > On Fri, May 27, 2011 at 7:53 PM, Stefano Sabatini > > <[email protected]> wrote: > > > +int av_parse_number(double *res, const char *numstr, enum > > > AVParseNumberType type, > > > + double min, double max, > > > + int log_offset, void *log_ctx) > > > +{ > > > + ParseUtils parseutils = { &parseutils_class, log_offset, log_ctx }; > > > + > > > + char *tail; > > > + double d = av_strtod(numstr, &tail); > > > + if ((unsigned)type >= AV_PARSE_NUM_TYPE_NB) { > > > + av_log(&parseutils, AV_LOG_ERROR, "Unknown parse number type > > > '%d'\n", type); > > > > This is a little bit of an ugly way to set up a logging context each > > time this is called. I admit this code isn't performance-critical, > > just looks a little ... Awkward. Can't we just pass it an existing > > logging context? The point of the log context is generally not to know > > where the error occurred (grep -r rocks), but rather to see what > > object it happened with. >
> I think those functions have way too many parameters to be considered > developer-friendly. If the problem is that the function has nothing > to log to -- well, then it shouldn't log at all. Just return > an appropriate error code and have the caller deal with it. Sometimes you may want to log, sometimes you need just to skip logging and avoid useless log spamming. In this specific case providing a logging context allows the application code to avoid to define a log message each time the function is called. The log_ctx+log_level_offset is the mechanism employed for such a scenario (e.g. libavutil/imgutils.c:av_image_check_size(), libavutil/eval.h), I agree that is a little awkward but at least from the API POV doesn't look that bad. And if the number of parameters is a problem you can either define a macro/convenience function in the application, either define them in the lib, e.g. av_parse_number_no_log(double *res, const char *numstr, enum AVParseNumberType type, double min, double max); but then you're trading function complexity with API complexity. _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
