On Sat, 28 May 2011 09:54:27 +0200, Stefano Sabatini <[email protected]> wrote: > 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.
I think that such a utility function shouldn't log anything at all ever. It's simply not its place to decide such things. I wouldn't expect av_malloc to log anything either. -- Anton Khirnov _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
