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

Message ID 20190415165700.22970-7-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • Rockchip ISP pipeline handler
Related show

Commit Message

Laurent Pinchart April 15, 2019, 4:56 p.m. UTC
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@ideasonboard.com>
---
 include/libcamera/geometry.h |  35 ++++++++++++
 src/libcamera/geometry.cpp   | 102 +++++++++++++++++++++++++++++++++++
 2 files changed, 137 insertions(+)

Comments

Niklas Söderlund April 15, 2019, 8:57 p.m. UTC | #1
Hi Laurent,

Thanks for your work.

On 2019-04-15 19:56:55 +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@ideasonboard.com>

I love to read repetitive documentation, you think you spot an error, 
reread it and it's once more correct. I did not managed to read it a 
third time and spot any errors,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Jacopo Mondi April 16, 2019, 3:09 p.m. UTC | #2
Hi Laurent,
   very nice, thanks

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@ideasonboard.com>

The only comment I have is that we usually put a blank line in
documentation between a \brief and a \return statement.

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

Minor stuff though:
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j
> ---
>  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
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart April 16, 2019, 8:08 p.m. UTC | #3
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@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@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 */
Niklas Söderlund April 16, 2019, 9:08 p.m. UTC | #4
Hello,

On 2019-04-16 23:08:38 +0300, Laurent Pinchart wrote:
> 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@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.
> 

I would support this format.
Jacopo Mondi April 17, 2019, 7:31 a.m. UTC | #5
Hi,

On Tue, Apr 16, 2019 at 11:08:16PM +0200, Niklas Söderlund wrote:
> Hello,
>
> On 2019-04-16 23:08:38 +0300, Laurent Pinchart wrote:
> > 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@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.
> >
>
> I would support this format.
>

I can't tell why, but I like the empty line before the \return.
Anyway, this is really not an issue worth discussing about, I welcome
any of the two if they are consistent though the code base.

Thanks
  j

> --
> Regards,
> Niklas Söderlund

Patch

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 */