[libcamera-devel] [PATCH 06/11] libcamera: geometry: Add comparison operators to geometry classes

[Laurent Pinchart]

Hi Jacopo,

On Tue, Apr 16, 2019 at 05:09:50PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>    very nice, thanks

Thank you :-)

> On Mon, Apr 15, 2019 at 07:56:55PM +0300, Laurent Pinchart wrote:
> > Add equality and inequality comparison operators for the Rectangle, Size
> > and SizeRange classes.
> >
> > For the Size class, also add ordering operators. Sizes are first
> > compared on combined width and height, then on area, and finally on
> > width only to achieve a stable ordering.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> 
> The only comment I have is that we usually put a blank line in
> documentation between a \brief and a \return statement.

Actually we don't in many cases. Both variants are used. I think it
would make sense to standardize on one of them, and I can send a patch
on top of this series. The question is, what should we standardize on ?
:-) I propose

/**
 * \fn
 * \brief
 * \param[in,out]
 *
 * Text
 *
 * \return
 */

\fn is required if the function is defined in the .cpp file and not
allowed otherwise. \brief is required. \param is required if the
function takes parameters, and the [in] and [out] specifiers are
required. The text is optional, and if present shall be surrounded by
empty lines. \return is required if the function returns a value and not
allowed otherwise.

This would mean no new line between \brief (or \param when present) and
\return if a long text is present.

> Also, "left hand Size" and "right hand Size" could be replaced by
> parameters name.

Good point. I'll do so.

> Minor stuff though:
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> 
> > ---
> >  include/libcamera/geometry.h |  35 ++++++++++++
> >  src/libcamera/geometry.cpp   | 102 +++++++++++++++++++++++++++++++++++
> >  2 files changed, 137 insertions(+)
> >
> > diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> > index 80f79c6ba2d3..3dbbced5c76f 100644
> > --- a/include/libcamera/geometry.h
> > +++ b/include/libcamera/geometry.h
> > @@ -21,6 +21,12 @@ struct Rectangle {
> >  	const std::string toString() const;
> >  };
> >
> > +bool operator==(const Rectangle &lhs, const Rectangle &rhs);
> > +static inline bool operator!=(const Rectangle &lhs, const Rectangle &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +
> >  struct Size {
> >  	Size()
> >  		: Size(0, 0)
> > @@ -36,6 +42,29 @@ struct Size {
> >  	unsigned int height;
> >  };
> >
> > +bool operator==(const Size &lhs, const Size &rhs);
> > +bool operator<(const Size &lhs, const Size &rhs);
> > +
> > +static inline bool operator!=(const Size &lhs, const Size &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +
> > +static inline bool operator<=(const Size &lhs, const Size &rhs)
> > +{
> > +	return lhs < rhs || lhs == rhs;
> > +}
> > +
> > +static inline bool operator>(const Size &lhs, const Size &rhs)
> > +{
> > +	return !(lhs <= rhs);
> > +}
> > +
> > +static inline bool operator>=(const Size &lhs, const Size &rhs)
> > +{
> > +	return !(lhs < rhs);
> > +}
> > +
> >  struct SizeRange {
> >  	SizeRange()
> >  	{
> > @@ -51,6 +80,12 @@ struct SizeRange {
> >  	Size max;
> >  };
> >
> > +bool operator==(const SizeRange &lhs, const SizeRange &rhs);
> > +static inline bool operator!=(const SizeRange &lhs, const SizeRange &rhs)
> > +{
> > +	return !(lhs == rhs);
> > +}
> > +
> >  } /* namespace libcamera */
> >
> >  #endif /* __LIBCAMERA_GEOMETRY_H__ */
> > diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> > index c1c7daed7259..7b65e63f2ebd 100644
> > --- a/src/libcamera/geometry.cpp
> > +++ b/src/libcamera/geometry.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <sstream>
> > +#include <stdint.h>
> >
> >  #include <libcamera/geometry.h>
> >
> > @@ -62,6 +63,22 @@ const std::string Rectangle::toString() const
> >  	return ss.str();
> >  }
> >
> > +/**
> > + * \brief Compare rectangles for equality
> > + * \return True if the two rectangles are equal, false otherwise
> > + */
> > +bool operator==(const Rectangle &lhs, const Rectangle &rhs)
> > +{
> > +	return lhs.x == rhs.x && lhs.y == rhs.y &&
> > +	       lhs.w == rhs.w && lhs.h == rhs.h;
> > +}
> > +
> > +/**
> > + * \fn bool operator!=(const Rectangle &lhs, const Rectangle &rhs)
> > + * \brief Compare rectangles for inequality
> > + * \return True if the two rectangles are not equal, false otherwise
> > + */
> > +
> >  /**
> >   * \struct Size
> >   * \brief Describe a two-dimensional size
> > @@ -91,6 +108,76 @@ const std::string Rectangle::toString() const
> >   * \brief The Size height
> >   */
> >
> > +/**
> > + * \brief Compare sizes for equality
> > + * \return True if the two sizes are equal, false otherwise
> > + */
> > +bool operator==(const Size &lhs, const Size &rhs)
> > +{
> > +	return lhs.width == rhs.width && lhs.height == rhs.height;
> > +}
> > +
> > +/**
> > + * \brief Compare sizes for smaller than order
> > + *
> > + * Sizes are compared on three criteria, in the following order.
> > + *
> > + * - A size with smaller width and smaller height is smaller.
> > + * - A size with smaller area is smaller.
> > + * - A size with smaller width is smaller.
> > + *
> > + * \return True if the left hand size is smaller than the right hand size, false
> > + * otherwise
> > + */
> > +bool operator<(const Size &lhs, const Size &rhs)
> > +{
> > +	if (lhs.width < rhs.width && lhs.height < rhs.height)
> > +		return true;
> > +	else if (lhs.width >= rhs.width && lhs.height >= rhs.height)
> > +		return false;
> > +
> > +	uint64_t larea = static_cast<uint64_t>(lhs.width) *
> > +			 static_cast<uint64_t>(lhs.height);
> > +	uint64_t rarea = static_cast<uint64_t>(rhs.width) *
> > +			 static_cast<uint64_t>(rhs.height);
> > +	if (larea < rarea)
> > +		return true;
> > +	else if (larea > rarea)
> > +		return false;
> > +
> > +	return lhs.width < rhs.width;
> > +}
> > +
> > +/**
> > + * \fn bool operator!=(const Size &lhs, const Size &rhs)
> > + * \brief Compare sizes for inequality
> > + * \return True if the two sizes are not equal, false otherwise
> > + */
> > +
> > +/**
> > + * \fn bool operator<=(const Size &lhs, const Size &rhs)
> > + * \brief Compare sizes for smaller than or equal to order
> > + * \return True if the left hand size is smaller than or equal to the right
> > + * hand size, false otherwise
> > + * \sa bool operator<(const Size &lhs, const Size &rhs)
> > + */
> > +
> > +/**
> > + * \fn bool operator>(const Size &lhs, const Size &rhs)
> > + * \brief Compare sizes for greater than order
> > + * \return True if the left hand size is greater than the right hand size,
> > + * false otherwise
> > + * \sa bool operator<(const Size &lhs, const Size &rhs)
> > + */
> > +
> > +/**
> > + * \fn bool operator>=(const Size &lhs, const Size &rhs)
> > + * \brief Compare sizes for greater than or equal to order
> > + * \return True if the left hand size is greater than or equal to the right
> > + * hand size, false otherwise
> > + * \sa bool operator<(const Size &lhs, const Size &rhs)
> > + */
> > +
> >  /**
> >   * \struct SizeRange
> >   * \brief Describe a range of sizes
> > @@ -124,4 +211,19 @@ const std::string Rectangle::toString() const
> >   * \brief The maximum size
> >   */
> >
> > +/**
> > + * \brief Compare size ranges for equality
> > + * \return True if the two size ranges are equal, false otherwise
> > + */
> > +bool operator==(const SizeRange &lhs, const SizeRange &rhs)
> > +{
> > +	return lhs.min == rhs.min && lhs.max == rhs.max;
> > +}
> > +
> > +/**
> > + * \fn bool operator!=(const SizeRange &lhs, const SizeRange &rhs)
> > + * \brief Compare size ranges for inequality
> > + * \return True if the two size ranges are not equal, false otherwise
> > + */
> > +
> >  } /* namespace libcamera */

-- 
Regards,

Laurent Pinchart