[FFmpeg-devel] [PATCH 2/8] lavu: new AVWriter API

Nicolas George george at nsup.org
Fri Apr 28 14:20:47 EEST 2023


Rodney Baker (12023-04-28):
> I'm not normally a reviewer, but I noticed a few minor grammatical things that 
> stood out - hope this is OK. 

Thanks, it is absolutely useful.

> Nit - s/compating/comparing/

Fixed.

> > +**Note:** AVWriter is 8-bit clean, the strings it manipulates can be
> Use a hyphen or a semicolon rather than a comma after "clean". 

After consideration, a semicolon would be too strong; a hyphen would be
strange, too literary.

> > +In mainstream C, a function that needs to return a string usually have two
> > +options: either they accept pointer to a buffer that they fill or they
> > +allocate the buffer themselves and return it. Both these options have
> > +drawbacks, which one is best depends on the circumstances of the caller.
> Semicolon instead of comma after "drawbacks".

Same here, I find a semicolon would be too strong.

> Drop comma after "circumstances". 

Done.

> > +AVWriter also makes the work of the called function easier by providing
> > +convenient functions to append to the string that completely wrap error
> > +checks. Note that it only works for strings created as streams; functions
> > +that need random access to the string already built still need to manage
> > +their own buffers; some AVWriter implementations can still help for that.
> Full stop after "buffers" (instead of semicolon - you've already used one 
> previously in the same sentence). 

Disagree on this one: the last part should still be in the sentence that
starts with “Note that”. There is no problem in having multiple
semicolons to separate parts on the same level.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20230428/48a6e7cf/attachment.sig>


More information about the ffmpeg-devel mailing list