[FFmpeg-devel] [PATCH] lavu: add escape API

[Stefano Sabatini]

On date Thursday 2013-01-24 18:01:31 +0100, Nicolas George encoded:
> Le duodi 22 nivôse, an CCXXI, Stefano Sabatini a écrit :
> > > av_bprint_finalize will fail if the buffer is truncated, so you do not have
> > > to check for it.
> > Not really from what I see.
> 
> You are right. I wonder if fixing it would be a good idea.
> 
> > What I'm trying to avoid is to make as hard as possible for the user
> > to get it wrong, the price to pay is a more complicated interface so
> > it is a tradeoff matter. I'll reconsider to merge the two fields.
> 
> My concern is that by adding a mode argument, you force the API user to make
> a choice, even if they do not want to because they only want whatever
> escaping lavu considers best.
> 
> > Truncation error is not considered an error here, invalid parameters
> > yes.
> 
> I consider there are two kinds of errors: those that can happen in a correct
> programs and the bugs, and I consider that the treatment for the two kinds
> of errors can be different.
> 
> Truncation of a bprint buffer can happen in a 100% correct program because
> it depends on outside conditions: memory availability. Therefore, the API
> must provide a means to check it.
> 
> Invalid flags, OTOH, are a programming error, a bug.
> 
> IMHO, the way of returning both kind of errors should be different; the
> simple C API does not allow that easily. But when a function can only fail
> due to a bug, never due to environmental circumstances, I find it better
> that it returns nothing. Reporting the bugs can be done another way, such as
> an assert failure or a log message.
> 
> In this particular case, I would find this completely acceptable:
> 
> 	default:
> 	    av_bprintf(dstbuf, "*** INVALID av_quote MODE %x ***", mode);
> 	    return;
> 
> But the best solution would still be to have no failure cases, i.e. have all
> flags configurations acceptable. For example, in the doxy:
> 
> 	* Any other value for mode will be considered equivalent to
> 	* AV_ESCAPE_MODE_BACKSLASH, but this behaviour can change without
> 	* notice.
> 
> >	Also in case more complex operations are performed (e.g. alloc)
> > it would be handy to be able to return an error.

Done like that.

> The principle in that case is to place the bprint buffer in a truncated
> state, so that the error can be detected at the end.
> 
> > >From f0d0f51ab59a0eaa2e27838afae963c96c1b02f1 Mon Sep 17 00:00:00 2001
> > From: Stefano Sabatini <stefasab at gmail.com>
> > Date: Sun, 16 Dec 2012 12:17:23 +0100
> > Subject: [PATCH] lavu: add escape API
> > 
> > The escape API will be useful to perform escaping programmatically, which
> > is required when crafting argument strings, and will be used for context
> > printing as well.
> > 
> > This is based on the ffescape tool code, with a few extensions and fixes.
> > 
> > TODO: bump minor, add APIchanges entry
[...]
> > +#define WHITESPACES " \n\t"
> > +
> > +int av_bprint_escape(AVBPrint *dstbuf, const char *src, const char *special_chars,
> > +                      enum AVEscapeMode mode, int flags)
> > +{
> > +    const char *src0 = src;
> > +
> > +    switch (mode) {
> > +    case AV_ESCAPE_MODE_BACKSLASH:
> > +        /* \-escape characters */
> 
> > +        for (; *src; src++) {
> > +            int is_first_last       = src == src0 || !*(src+1);
> > +            int is_ws               = !!strchr(WHITESPACES, *src);
> > +            int is_strictly_special = special_chars && strchr(special_chars, *src);
> > +            int is_special          =
> > +                is_strictly_special || strchr("'\\", *src) ||
> > +                (is_ws && (flags & AV_ESCAPE_FLAG_WHITESPACE));
> > +
> > +            if (is_strictly_special ||
> > +                (!(flags & AV_ESCAPE_FLAG_STRICT) &&
> > +                 (is_special || (is_ws && is_first_last))))
> > +                av_bprint_chars(dstbuf, '\\', 1);
> > +            av_bprint_chars(dstbuf, *src, 1);
> 
> Looks nice.

Well nice is not the right term, it is the less ugly thing I could
come with.

> Did you look at the assembly code for the strchr("'\\") bit?

No, why?

[...] 
> No other remarks this time.

Updated.
-- 
FFmpeg = Fierce and Fundamental Magnificient Political Exxagerate Gospel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-lavu-add-escape-API.patch
Type: text/x-diff
Size: 12312 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130221/7d851f0d/attachment.bin>