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.

Incomputable · Accepted Answer · 2021-12-25 12:37:46Z

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 assert expression will become noop if NDEBUG is defined, which is usually the case for CMake's default Release build type for example. — Incomputable, Commented Jan 27 at 22:22

Stack Exchange Network

Operator overloading in Image class implementation in C++

1 Answer 1

Build script review

Avoid `CMAKE_CXX_FLAGS`, `include_dirs` and other globals

Use Threads package instead of pthreads

Use generator expressions where appropriate

Code Review

Don't move from const variable

Don't forget noexcept

Subscript operator conventions

Benchmark? review

Understand performance metrics

Run it multiple times

Be careful with clocks

Vector instructions

Your Answer

Not the answer you're looking for? Browse other questions tagged
c++
unit-testing
classes
c++20
overloading
or ask your own question.

Linked

Hot Network Questions

Operator overloading in Image class implementation in C++

1 Answer 1

Build script review

Avoid CMAKE_CXX_FLAGS, include_dirs and other globals

Use Threads package instead of pthreads

Use generator expressions where appropriate

Code Review

Don't move from const variable

Don't forget noexcept

Subscript operator conventions

Benchmark? review

Understand performance metrics

Run it multiple times

Be careful with clocks

Vector instructions

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged c++unit-testingclassesc++20overloading or ask your own question.

Linked

Related

Hot Network Questions

Avoid `CMAKE_CXX_FLAGS`, `include_dirs` and other globals

Not the answer you're looking for? Browse other questions tagged
c++
unit-testing
classes
c++20
overloading
or ask your own question.