[Ffmpeg-devel] [RFC][PATCH]Doxygenize libavutil/fifo.h

Diego Biurrun diego
Fri Mar 2 11:12:50 CET 2007


On Tue, Feb 27, 2007 at 10:27:35PM +0100, Michael Niedermayer wrote:
> 
> > --- fifo.h	(revision 8149)
> > +++ fifo.h	(working copy)
> > @@ -24,15 +29,74 @@
> > + * Initializes a FIFO *.
> 
> initalizes an AVFifoBuffer
> also the function does not initalize a pointer to a fifo but the fifo struct
> itself so the * is not appropriate
> 
> > + * @param *f fifo buffer
> 
> @param f the AVFifoBuffer to initalize
> 
> > + * Frees a FIFO *.
> 
> frees an AVFifoBuffer
> 
> > + * @param *f fifo buffer
> 
> @param f the AVFifoBuffer to free
> 
> > + * Gets the size of a FIFO *.
> 
> what is the size of a fifo? this is unlear, if you dont know what a function
> does then dont add a comment to it but leave it to someone else to do who does
> goal is not to have as many comments as possible but to provide helpfull
> information to the reader
> 
> correct here is
> gets the amount of data in bytes in the fifo, that is the amount
> you could read from it
> 
> > + * Reads the data from the FIFO *.
> > + * @param *f fifo buffer
> > + * @param *buf data destination
> > + * @param buf_size of data
> 
> @param buf_size number of bytes to read
> 
> > + * @return -1 if not enough data
> 
> ive already said that implementation details like -1 return are unaccptable
> <0 is error >= 0 is no error
> the doxygen comments in the header are a API specification they must be
> written carefully, errors are totally unacceptable its much better if
> something is undocumented
> 
> >  int av_fifo_read(AVFifoBuffer *f, uint8_t *buf, int buf_size);
> > +
> > +/**
> > + * Reads the data from the FIFO *.
> 
> again dont write half correct comments, the comment is the same as for
> the last but the function does not do the same
> 
> it rather feeds data from the fifo to a user supplied callback while
> the previous reads data into a buffer
> 
> > + * @param *f fifo buffer
> > + * @param buf_size data size
> > + * @param *func generic read function
> > + * @param *dest data destination
> 
> > + * @return -1 if not enough data
> 
> implementation detail
> 
> > + * Writes the data in the FIFO *.
> 
> "in" is wrong use "into"
> 
> > + * Discards the data from the FIFO *.
> 
> reads and descards the specified amount of data from the AVFifoBuffer
> 
> > + * Returns a pointer with circular offset from FIFO's read pointer.
> > + * @param *f fifo buffer
> > + * @param offs offset
> > + * @return ptr=rptr+offs if rptr+offs<end else rptr+offs -(end-begin)
> > + */
> >  static inline uint8_t av_fifo_peek(AVFifoBuffer *f, int offs)
> 
> this function should be removed as it exposes implemenbtation details
> and should thus not be documented

Should all be fixed in my latest commit, please review what's in SVN, if
it still has shortcomings, I'll address them.

Diego




More information about the ffmpeg-devel mailing list