Operator overloading in Image class implementation in C++

Question

This is a follow-up question for Dictionary based non-local mean implementation in C++. There are some issues about operators (operator+ and operator-) mentioned by Edward's answer and JDługosz's comment. I am trying to propose the fixed version of Image class as below.

• operator+ and operator- operator overloading:

  friend Image<ElementT> operator+(Image<ElementT> input1, const Image<ElementT>& input2)
  {
   return input1 += input2;
  }
  friend Image<ElementT> operator-(Image<ElementT> input1, const Image<ElementT>& input2)
  {
   return input1 -= input2;
  }
  

• The full Image class implementation:

  template <typename ElementT>
  class Image
  {
  public:
   Image() = default;
   Image(const std::size_t width, const std::size_t height):
   width(width),
   height(height),
   image_data(width * height) { }
   Image(const std::size_t width, const std::size_t height, const ElementT initVal):
   width(width),
   height(height),
   image_data(width * height, initVal) {}
   Image(const std::vector<ElementT> input, std::size_t newWidth, std::size_t newHeight):
   width(newWidth),
   height(newHeight)
   {
   if (input.size() != newWidth * newHeight)
   {
   throw std::runtime_error("Image data input and the given size are mismatched!");
   }
   image_data = std::move(input);
   }
   constexpr ElementT& at(const unsigned int x, const unsigned int y)
   { 
   checkBoundary(x, y);
   return image_data[y * width + x];
   }
   constexpr ElementT const& at(const unsigned int x, const unsigned int y) const
   {
   checkBoundary(x, y);
   return image_data[y * width + x];
   }
   constexpr std::size_t getWidth() const
   {
   return width;
   }
   constexpr std::size_t getHeight() const
   {
   return height;
   }
   constexpr auto getSize()
   {
   return std::make_tuple(width, height);
   }
   std::vector<ElementT> const& getImageData() const { return image_data; } // expose the internal data
   void print(std::string separator = "\t", std::ostream& os = std::cout) const
   {
   for (std::size_t y = 0; y < height; ++y)
   {
   for (std::size_t x = 0; x < width; ++x)
   {
   // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
   os << +at(x, y) << separator;
   }
   os << "\n";
   }
   os << "\n";
   return;
   }
   // Enable this function if ElementT = RGB
   void print(std::string separator = "\t", std::ostream& os = std::cout) const
   requires(std::same_as<ElementT, RGB>)
   {
   for (std::size_t y = 0; y < height; ++y)
   {
   for (std::size_t x = 0; x < width; ++x)
   {
   os << "( ";
   for (std::size_t channel_index = 0; channel_index < 3; ++channel_index)
   {
   // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
   os << +at(x, y).channels[channel_index] << separator;
   }
   os << ")" << separator;
   }
   os << "\n";
   }
   os << "\n";
   return;
   }
   friend std::ostream& operator<<(std::ostream& os, const Image<ElementT>& rhs)
   {
   const std::string separator = "\t";
   for (std::size_t y = 0; y < rhs.height; ++y)
   {
   for (std::size_t x = 0; x < rhs.width; ++x)
   {
   // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
   os << +rhs.at(x, y) << separator;
   }
   os << "\n";
   }
   os << "\n";
   return os;
   }
   Image<ElementT>& operator+=(const Image<ElementT>& rhs)
   {
   assert(rhs.width == this->width);
   assert(rhs.height == this->height);
   std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
   std::ranges::begin(image_data), std::plus<>{});
   return *this;
   }
   Image<ElementT>& operator-=(const Image<ElementT>& rhs)
   {
   assert(rhs.width == this->width);
   assert(rhs.height == this->height);
   std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
   std::ranges::begin(image_data), std::minus<>{});
   return *this;
   }
   Image<ElementT>& operator*=(const Image<ElementT>& rhs)
   {
   assert(rhs.width == this->width);
   assert(rhs.height == this->height);
   std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
   std::ranges::begin(image_data), std::multiplies<>{});
   return *this;
   }
   Image<ElementT>& operator/=(const Image<ElementT>& rhs)
   {
   assert(rhs.width == this->width);
   assert(rhs.height == this->height);
   std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
   std::ranges::begin(image_data), std::divides<>{});
   return *this;
   }
   friend bool operator==(Image<ElementT> const&, Image<ElementT> const&) = default;
   friend bool operator!=(Image<ElementT> const&, Image<ElementT> const&) = default;
   friend Image<ElementT> operator+(Image<ElementT> input1, const Image<ElementT>& input2)
   {
   return input1 += input2;
   }
   friend Image<ElementT> operator-(Image<ElementT> input1, const Image<ElementT>& input2)
   {
   return input1 -= input2;
   }
   Image<ElementT>& operator=(Image<ElementT> const& input) = default; // Copy Assign
   Image<ElementT>& operator=(Image<ElementT>&& other) = default; // Move Assign
   Image(const Image<ElementT> &input) = default; // Copy Constructor
   Image(Image<ElementT> &&input) = default; // Move Constructor
  private:
   std::size_t width;
   std::size_t height;
   std::vector<ElementT> image_data;
   void checkBoundary(const size_t x, const size_t y) const
   {
   assert(x < width);
   assert(y < height);
   }
  };
  

Unit tests for operator+and operator-:

void operatorAddInImageClassTest(const std::size_t sizex, const std::size_t sizey)
{
 // Assign
 auto test_image1 = TinyDIP::Image(sizex, sizey, 1);
 auto test_image2 = TinyDIP::Image(sizex, sizey, 2);
 // Action
 auto actual_result = test_image1 + test_image2;
 // Assert
 auto expected_result = TinyDIP::Image(sizex, sizey, 3);
 assert(actual_result == expected_result);
 return;
}
void operatorMinusInImageClassTest(const std::size_t sizex, const std::size_t sizey)
{
 // Assign
 auto test_image1 = TinyDIP::Image(sizex, sizey, 1);
 auto test_image2 = TinyDIP::Image(sizex, sizey, 2);
 // Action
 auto actual_result = test_image1 - test_image2;
 // Assert
 auto expected_result = TinyDIP::Image(sizex, sizey, -1);
 assert(actual_result == expected_result);
 return;
}

Full Testing Code

The full tests for operator+and operator-:

#include <algorithm>
#include <array>
#include <cassert>
#include <chrono>
#include <cmath>
#include <concepts>
#include <exception>
#include <fstream>
#include <iostream>
#include <limits>
#include <numeric>
#include <ranges>
#include <type_traits>
#include <utility>
#include <vector>
using BYTE = unsigned char;
struct RGB
{
 BYTE channels[3];
};
using GrayScale = BYTE;
namespace TinyDIP
{
 #define is_size_same(x, y) {assert(x.getWidth() == y.getWidth()); assert(x.getHeight() == y.getHeight());}
 template <typename ElementT>
 class Image
 {
 public:
 Image() = default;
 Image(const std::size_t width, const std::size_t height):
 width(width),
 height(height),
 image_data(width * height) { }
 Image(const std::size_t width, const std::size_t height, const ElementT initVal):
 width(width),
 height(height),
 image_data(width * height, initVal) {}
 Image(const std::vector<ElementT> input, std::size_t newWidth, std::size_t newHeight):
 width(newWidth),
 height(newHeight)
 {
 if (input.size() != newWidth * newHeight)
 {
 throw std::runtime_error("Image data input and the given size are mismatched!");
 }
 image_data = std::move(input);
 }
 constexpr ElementT& at(const unsigned int x, const unsigned int y)
 { 
 checkBoundary(x, y);
 return image_data[y * width + x];
 }
 constexpr ElementT const& at(const unsigned int x, const unsigned int y) const
 {
 checkBoundary(x, y);
 return image_data[y * width + x];
 }
 constexpr std::size_t getWidth() const
 {
 return width;
 }
 constexpr std::size_t getHeight() const
 {
 return height;
 }
 constexpr auto getSize()
 {
 return std::make_tuple(width, height);
 }
 std::vector<ElementT> const& getImageData() const { return image_data; } // expose the internal data
 void print(std::string separator = "\t", std::ostream& os = std::cout) const
 {
 for (std::size_t y = 0; y < height; ++y)
 {
 for (std::size_t x = 0; x < width; ++x)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +at(x, y) << separator;
 }
 os << "\n";
 }
 os << "\n";
 return;
 }
 // Enable this function if ElementT = RGB
 void print(std::string separator = "\t", std::ostream& os = std::cout) const
 requires(std::same_as<ElementT, RGB>)
 {
 for (std::size_t y = 0; y < height; ++y)
 {
 for (std::size_t x = 0; x < width; ++x)
 {
 os << "( ";
 for (std::size_t channel_index = 0; channel_index < 3; ++channel_index)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +at(x, y).channels[channel_index] << separator;
 }
 os << ")" << separator;
 }
 os << "\n";
 }
 os << "\n";
 return;
 }
 friend std::ostream& operator<<(std::ostream& os, const Image<ElementT>& rhs)
 {
 const std::string separator = "\t";
 for (std::size_t y = 0; y < rhs.height; ++y)
 {
 for (std::size_t x = 0; x < rhs.width; ++x)
 {
 // Ref: https://isocpp.org/wiki/faq/input-output#print-char-or-ptr-as-number
 os << +rhs.at(x, y) << separator;
 }
 os << "\n";
 }
 os << "\n";
 return os;
 }
 Image<ElementT>& operator+=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
 std::ranges::begin(image_data), std::plus<>{});
 return *this;
 }
 Image<ElementT>& operator-=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
 std::ranges::begin(image_data), std::minus<>{});
 return *this;
 }
 Image<ElementT>& operator*=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
 std::ranges::begin(image_data), std::multiplies<>{});
 return *this;
 }
 Image<ElementT>& operator/=(const Image<ElementT>& rhs)
 {
 assert(rhs.width == this->width);
 assert(rhs.height == this->height);
 std::transform(std::ranges::cbegin(image_data), std::ranges::cend(image_data), std::ranges::cbegin(rhs.image_data),
 std::ranges::begin(image_data), std::divides<>{});
 return *this;
 }
 friend bool operator==(Image<ElementT> const&, Image<ElementT> const&) = default;
 friend bool operator!=(Image<ElementT> const&, Image<ElementT> const&) = default;
 friend Image<ElementT> operator+(Image<ElementT> input1, const Image<ElementT>& input2)
 {
 return input1 += input2;
 }
 friend Image<ElementT> operator-(Image<ElementT> input1, const Image<ElementT>& input2)
 {
 return input1 -= input2;
 }
 Image<ElementT>& operator=(Image<ElementT> const& input) = default; // Copy Assign
 Image<ElementT>& operator=(Image<ElementT>&& other) = default; // Move Assign
 Image(const Image<ElementT> &input) = default; // Copy Constructor
 Image(Image<ElementT> &&input) = default; // Move Constructor
 
 private:
 std::size_t width;
 std::size_t height;
 std::vector<ElementT> image_data;
 void checkBoundary(const size_t x, const size_t y) const
 {
 assert(x < width);
 assert(y < height);
 }
 };
}
void operatorAddInImageClassTest(const std::size_t sizex, const std::size_t sizey)
{
 // Assign
 auto test_image1 = TinyDIP::Image(sizex, sizey, 1);
 auto test_image2 = TinyDIP::Image(sizex, sizey, 2);
 // Action
 auto actual_result = test_image1 + test_image2;
 // Assert
 auto expected_result = TinyDIP::Image(sizex, sizey, 3);
 assert(actual_result == expected_result);
 return;
}
void operatorMinusInImageClassTest(const std::size_t sizex, const std::size_t sizey)
{
 // Assign
 auto test_image1 = TinyDIP::Image(sizex, sizey, 1);
 auto test_image2 = TinyDIP::Image(sizex, sizey, 2);
 // Action
 auto actual_result = test_image1 - test_image2;
 // Assert
 auto expected_result = TinyDIP::Image(sizex, sizey, -1);
 assert(actual_result == expected_result);
 return;
}
int main()
{
 auto start = std::chrono::system_clock::now();
 operatorAddInImageClassTest(10, 10);
 operatorMinusInImageClassTest(10, 10);
 auto max_size = std::numeric_limits<std::size_t>::max();
 operatorAddInImageClassTest(max_size, max_size);
 operatorMinusInImageClassTest(max_size, max_size);
 auto end = std::chrono::system_clock::now();
 std::chrono::duration<double> elapsed_seconds = end - start;
 std::time_t end_time = std::chrono::system_clock::to_time_t(end);
 std::cout << "Computation finished at " << std::ctime(&end_time) << "elapsed time: " << elapsed_seconds.count() << '\n';
 return 0;
}

The output of the testing code above:

Computation finished at Sat Dec 25 07:08:32 2021
elapsed time: 1.2452e-05

A Godbolt link is here.

TinyDIP on GitHub

All suggestions are welcome.

The summary information:

• Which question it is a follow-up to?

  Dictionary based non-local mean implementation in C++

• What changes has been made in the code since last question?

  Update and fix the operator+ and operator- implementation in Image class.

• Why a new review is being asked for?

  If there is any possible improvement, please let me know.

Answer 1

Build script review

Build scripts are major part of the software, so I will review it along with the main code.

Avoid CMAKE_CXX_FLAGS, include_dirs and other globals

There is target_compile_options to not make things global. Optimization flags are better ommitted altogether.

The testing code uses assert, but build script always specifies optimization flag. Are tests really not failing? (perhaps the script was modified after testing)

Use Threads package instead of pthreads

find_package(Threads REQUIRED)
target_link_libraries(TARGET PUBLIC Threads::Threads)

I believe there was something similar to filesystem, but I couldn't find it. Unfortunately libm does have better linking mechanism.

Use generator expressions where appropriate

There are some pieces with if() else() chains. It might be better to use generator expressions in those cases. Since generator expression becomes nothing if not evaluated to 1, it just becomes empty (not even a space, literally empty, IIRC). This matches the build script pieces where some branches empty and some branches are not (there is a generator expression for compiler ID).

---

Code Review

Don't move from const variable

image_data = std::move(input);

This will not move from input because input is const and overload resolution will lead to copy. I guess originally it was a reference, then it was migrated into copy and move idiom. Just drop the const from the declaration.

Don't forget noexcept

There are multiple one-liners that clearly don't throw exceptions

Subscript operator conventions

My initial expectation was to search for operator()(std::size_t, std::size_t). In all of the BLAS-like libraries I used, the function call operator was the subscript operator. at() implies always-on bound checking, but that is not the case. Although this is just a convention, it might surprise users.

---

Benchmark? review

Not sure if the tests were a benchmark.

Understand performance metrics

It is important to know what is being measured and why it is being measured. The current code seems to be to just "gauge" at how it behaves. Perhaps it would be better to pit it against OpenCV. The heavy hitters like MKL, Blaze, Eigen and uBLAS would be great too. Don't expect the compiler generated code to be as fast as those libraries, but it might provide good insight into future considerations.

Run it multiple times

The first run might step on page faults and OS/runtime facility initialization. There might be context switches (one context switch can skew results tremendeously).

Be careful with clocks

Non-monotonic clocks have a bad habit of getting calibrated in the most inopportune moments.

---

Vector instructions

It might make sense to examine the assembly output to see if there are any AVX instructions, at least AVX2. If there are none, it might be worth it to have a look at vectorization libraries.

• JimmyHu: Will assert clause be passed with optimization flag?
• Incomputable: @JimmyHu assert expression will become noop if NDEBUG is defined, which is usually the case for CMake's default Release build type for example.