On date Saturday 2011-05-28 11:23:50 +0200, Anton Khirnov wrote: > 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. Logging in the called function code has two advantages: * you don't have to log the same message again and again in application code, in case of failure * the function code knows exactly why the failure happened, and can provide useful hints for spotting the problem, which can't be done in the calling code. libav* adopts both logging and non-logging policy, in general it makes sense to define a log context when there is something meaningful to say and/or you don't want to repeat the message in application code. _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
