[libcamera-devel] [PATCH v2] libcamera: Add operator<< for classes in geometry

[Kieran Bingham]

Hi Han-lin,

Quoting Han-Lin Chen via libcamera-devel (2022-04-18 13:19:09)
> Add operator<< for geometry classes for easier logging.
> 

I can't remember which thread I said it on - but yes I like this ;-)
I'd rather use << object << than << object.toString() << in debug.

I think it produces more idiomatic C++.


> Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> ---
>  include/libcamera/geometry.h |  8 +++++
>  src/libcamera/geometry.cpp   | 57 +++++++++++++++++++++++++++++++-----
>  2 files changed, 57 insertions(+), 8 deletions(-)
> 
> diff --git a/include/libcamera/geometry.h b/include/libcamera/geometry.h
> index 7838b679..d4a144bc 100644
> --- a/include/libcamera/geometry.h
> +++ b/include/libcamera/geometry.h
> @@ -46,6 +46,8 @@ static inline bool operator!=(const Point &lhs, const Point &rhs)
>         return !(lhs == rhs);
>  }
>  
> +std::ostream &operator<<(std::ostream &out, const Point &p);
> +
>  class Size
>  {
>  public:
> @@ -192,6 +194,8 @@ static inline bool operator>=(const Size &lhs, const Size &rhs)
>         return !(lhs < rhs);
>  }
>  
> +std::ostream &operator<<(std::ostream &out, const Size &s);
> +
>  class SizeRange
>  {
>  public:
> @@ -232,6 +236,8 @@ static inline bool operator!=(const SizeRange &lhs, const SizeRange &rhs)
>         return !(lhs == rhs);
>  }
>  
> +std::ostream &operator<<(std::ostream &out, const SizeRange &sr);
> +
>  class Rectangle
>  {
>  public:
> @@ -291,4 +297,6 @@ static inline bool operator!=(const Rectangle &lhs, const Rectangle &rhs)
>         return !(lhs == rhs);
>  }
>  
> +std::ostream &operator<<(std::ostream &out, const Rectangle &r);
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/geometry.cpp b/src/libcamera/geometry.cpp
> index cb3c2de1..a0696c51 100644
> --- a/src/libcamera/geometry.cpp
> +++ b/src/libcamera/geometry.cpp
> @@ -56,8 +56,7 @@ namespace libcamera {
>  const std::string Point::toString() const
>  {
>         std::stringstream ss;
> -
> -       ss << "(" << x << "," << y << ")";
> +       ss << *this;
>  
>         return ss.str();
>  }
> @@ -83,6 +82,16 @@ bool operator==(const Point &lhs, const Point &rhs)
>   * \return True if the two points are not equal, false otherwise
>   */
>  
> +/**
> + * \brief Insert operation for Point with ostream

Aha - I was going to complain here, that it sounds like an instruction
'we will insert an operation here for Point with ostream' - but I've
just learnt that operator<< is the insertion operator. (And >> is the
'extraction' operator).

I might suggest a small addition to make the sentence more complete:

"Insert operation for a Point with an ostream"

But it's optional - would apply to other uses too of course.


> + * \return The input std::ostream

'The input ostream' sounds odd here.

How about

\return The output stream \a out

> + */
> +std::ostream &operator<<(std::ostream &out, const Point &p)
> +{
> +       out << "(" << p.x << ", " << p.y << ")";
> +       return out;
> +}
> +
>  /**
>   * \struct Size
>   * \brief Describe a two-dimensional size
> @@ -124,7 +133,10 @@ bool operator==(const Point &lhs, const Point &rhs)
>   */
>  const std::string Size::toString() const
>  {
> -       return std::to_string(width) + "x" + std::to_string(height);
> +       std::stringstream ss;
> +       ss << *this;
> +
> +       return ss.str();
>  }
>  
>  /**
> @@ -428,6 +440,16 @@ bool operator<(const Size &lhs, const Size &rhs)
>   * \sa bool operator<(const Size &lhs, const Size &rhs)
>   */
>  
> +/**
> + * \brief Insert operation for Size with ostream
> + * \return The input std::ostream
> + */
> +std::ostream &operator<<(std::ostream &out, const Size &s)
> +{
> +       out << s.width << "x" << s.height;
> +       return out;
> +}
> +
>  /**
>   * \struct SizeRange
>   * \brief Describe a range of sizes
> @@ -528,9 +550,7 @@ bool SizeRange::contains(const Size &size) const
>  std::string SizeRange::toString() const
>  {
>         std::stringstream ss;
> -
> -       ss << "(" << min.toString() << ")-(" << max.toString() << ")/(+"
> -          << hStep << ",+" << vStep << ")";
> +       ss << *this;
>  
>         return ss.str();
>  }
> @@ -550,6 +570,18 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
>   * \return True if the two size ranges are not equal, false otherwise
>   */
>  
> +/**
> + * \brief Insert operation for SizeRange with ostream
> + * \return The input std::ostream
> + */
> +std::ostream &operator<<(std::ostream &out, const SizeRange &sr)
> +{
> +       out << "(" << sr.min << ")-(" << sr.max << ")/(+"
> +           << sr.hStep << ",+" << sr.vStep << ")";
> +
> +       return out;
> +}
> +
>  /**
>   * \struct Rectangle
>   * \brief Describe a rectangle's position and dimensions
> @@ -624,8 +656,7 @@ bool operator==(const SizeRange &lhs, const SizeRange &rhs)
>  const std::string Rectangle::toString() const
>  {
>         std::stringstream ss;
> -
> -       ss << "(" << x << "x" << y << ")/" << width << "x" << height;

I see this is changed to a ','. That sounds like a bug fix, which is
correct - but shouldn't be hidden in this large patch.  Please split it
to either before or after this patch.


With all of that I think you can also add this to your next revision

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +       ss << *this;
>  
>         return ss.str();
>  }
> @@ -796,4 +827,14 @@ bool operator==(const Rectangle &lhs, const Rectangle &rhs)
>   * \return True if the two rectangles are not equal, false otherwise
>   */
>  
> +/**
> + * \brief Insert operation for Rectangle with ostream
> + * \return The input std::ostream
> + */
> +std::ostream &operator<<(std::ostream &out, const Rectangle &r)
> +{
> +       out << "(" << r.x << ", " << r.y << ")/" << r.width << "x" << r.height;
> +       return out;
> +}
> +
>  } /* namespace libcamera */
> -- 
> 2.36.0.rc0.470.gd361397f0d-goog
>