2
\$\begingroup\$

I want to implement color space logic.

So I started with creating some structs:

struct RGB {
    unsigned char r,g,b;
};

struct ARGB {
    unsigned char a,r,g,b;
};

struct HSL {
    float h,s,l;
};

struct Lab {
    float l,a,b;
};

As you can see, I have different color spaces in my program: the RGB and CIEXYZ. Then I realized that I have to do operator overloading for all of the structs (color models), so I started thinking about creating a base struct ColorModel which has to somehow do all the operator overloading. But I can't have any virtual functions => pointers to vtable, as it will increase the size of any pixel and imagine what will be the size of a picture in my ram? So I decided to do some template metaprogramming, CRTP to be more specific. I created my base struct like this:

#include <iostream>
#include <algorithm>
#include <array>

namespace color_space {
template<int Space, typename T, typename C>
struct ColorModel {
    
    static constexpr int SPACE = Space;
    
    ColorModel() {
        std::fill(begin(), end(), 0);
    };
    
    ColorModel(const ColorModel& other) {
        std::copy(other.begin(), other.end(), begin());
    }
    
    ColorModel(ColorModel&& other) noexcept {
        std::move(other.begin(), other.end(), begin());
    }
    
    ColorModel(const std::initializer_list<T>& l) {
        std::copy(l.begin(), l.end(), begin());
    }
    
    explicit ColorModel(const T& elem) {
        std::fill(begin(), end(), elem);
    }
    
    const T* cbegin() const {
        return static_cast<const C*>(this)->components.cbegin();
    }
    
    const T* cend() const {
        return static_cast<const C*>(this)->components.cend();
    }
    
    const T* begin() const {
        return cbegin();
    }
    
    const T* end() const {
        return cend();
    }
    
    T* begin() {
        return static_cast<C*>(this)->components.begin();
    }
    
    T* end() {
        return static_cast<C*>(this)->components.end();
    }
    
    size_t size() const {
        return std::distance(begin(), end());
    }
    
    const T& operator[](unsigned int i) const {
        return *(begin() + i);
    }
    
    T& operator[](unsigned int i) {
        return *(begin() + i);
    }
    
    ColorModel& operator=(const ColorModel& rhs) {
        std::copy(rhs.begin(), rhs.end(), begin());
        return *this;
    }
    
    ColorModel& operator=(ColorModel&& rhs) noexcept {
        std::move(rhs.begin(), rhs.end(), begin());
        return *this;
    }
    
    ColorModel& operator=(const T& rhs) {
        std::fill(begin(), end(), rhs);
        return *this;
    }
    
    ColorModel& operator=(T&& rhs) noexcept {
        std::fill(begin(), end(), rhs);
        return *this;
    }
    
    ColorModel& operator+=(const ColorModel& rhs) {
        std::transform(begin(), end(), rhs.begin(), begin(), std::plus<>());
        return *this;
    }
    
    ColorModel& operator+=(ColorModel&& rhs) {
        std::transform(begin(), end(), rhs.begin(), begin(), std::plus<>());
        return *this;
    }
    
    ColorModel& operator+=(const T& rhs) {
        std::for_each(begin(), end(), [&](T& elem) { elem += rhs; });
        return *this;
    }
    
    ColorModel& operator+=(T&& rhs) {
        std::for_each(begin(), end(), [&](T& elem) { elem += rhs; });
        return *this;
    }
    
    ColorModel& operator-=(const ColorModel& rhs) {
        std::transform(begin(), end(), rhs.begin(), begin(), std::minus<>());
        return *this;
    }
    
    ColorModel& operator-=(ColorModel&& rhs) {
        std::transform(begin(), end(), rhs.begin(), begin(), std::minus<>());
        return *this;
    }
    
    ColorModel& operator-=(const T& rhs) {
        std::for_each(begin(), end(), [&](T& elem) { elem -= rhs; });
        return *this;
    }
    
    ColorModel& operator-=(T&& rhs) {
        std::for_each(begin(), end(), [&](T& elem) { elem -= rhs; });
        return *this;
    }
    
    ColorModel& operator*=(const ColorModel& rhs) {
        std::transform(begin(), end(), rhs.begin(), begin(), std::multiplies<>());
        return *this;
    }
    
    ColorModel& operator*=(ColorModel&& rhs) {
        std::transform(begin(), end(), rhs.begin(), begin(), std::multiplies<>());
        return *this;
    }
    
    ColorModel& operator*=(const T& rhs) {
        std::for_each(begin(), end(), [&](T& elem) { elem *= rhs; });
        return *this;
    }
    
    ColorModel& operator*=(T&& rhs) {
        std::for_each(begin(), end(), [&](T& elem) { elem *= rhs; });
        return *this;
    }
    
    ColorModel& operator/=(const ColorModel& rhs) {
        std::transform(begin(), end(), rhs.begin(), begin(), std::divides<>());
        return *this;
    }
    
    ColorModel& operator/=(ColorModel&& rhs) {
        std::transform(begin(), end(), rhs.begin(), begin(), std::divides<>());
        return *this;
    }
    
    ColorModel& operator/=(const T& rhs) {
        std::for_each(begin(), end(), [&](T& elem) { elem /= rhs; });
        return *this;
    }
    
    ColorModel& operator/=(T&& rhs) {
        std::for_each(begin(), end(), [&](T& elem) { elem /= rhs; });
        return *this;
    }
    
    ColorModel operator+(const ColorModel& rhs) const {
        ColorModel result;
        std::transform(begin(), end(), rhs.begin(), result.begin(), std::plus<>());
        return result;
    }
    
    ColorModel operator+(ColorModel&& rhs) const {
        ColorModel result;
        std::transform(begin(), end(), rhs.begin(), result.begin(), std::plus<>());
        return result;
    }
    
    ColorModel operator+(const T& rhs) const {
        ColorModel result;
#pragma clang loop vectorize(enable)
        for (int i = 0; i < size(); ++i) {
            result[i] = (*this)[i] + rhs;
        }
        return result;
    }
    
    ColorModel operator+(T&& rhs) const {
        ColorModel result;
#pragma clang loop vectorize(enable)
        for (int i = 0; i < size(); ++i) {
            result[i] = (*this)[i] + rhs;
        }
        return result;
    }
    
    ColorModel operator-(const ColorModel& rhs) const {
        ColorModel result;
        std::transform(begin(), end(), rhs.begin(), result.begin(), std::minus<>());
        return result;
    }
    
    ColorModel operator-(ColorModel&& rhs) const {
        ColorModel result;
        std::transform(begin(), end(), rhs.begin(), result.begin(), std::minus<>());
        return result;
    }
    
    ColorModel operator-(const T& rhs) const {
        ColorModel result;
#pragma clang loop vectorize(enable)
        for (int i = 0; i < size(); ++i) {
            result[i] = (*this)[i] - rhs;
        }
        return result;
    }
    
    ColorModel operator-(T&& rhs) const {
        ColorModel result;
#pragma clang loop vectorize(enable)
        for (int i = 0; i < size(); ++i) {
            result[i] = (*this)[i] - rhs;
        }
        return result;
    }
    
    ColorModel operator*(const ColorModel& rhs) const {
        ColorModel result;
        std::transform(begin(), end(), rhs.begin(), result.begin(), std::multiplies<>());
        return result;
    }
    
    ColorModel operator*(ColorModel&& rhs) const {
        ColorModel result;
        std::transform(begin(), end(), rhs.begin(), result.begin(), std::multiplies<>());
        return result;
    }
    
    ColorModel operator*(const T& rhs) const {
        ColorModel result;
#pragma clang loop vectorize(enable)
        for (int i = 0; i < size(); ++i) {
            result[i] = (*this)[i] * rhs;
        }
        return result;
    }
    
    ColorModel operator*(T&& rhs) const {
        ColorModel result;
#pragma clang loop vectorize(enable)
        for (int i = 0; i < size(); ++i) {
            result[i] = (*this)[i] * rhs;
        }
        return result;
    }
    
    ColorModel operator/(const ColorModel& rhs) const {
        ColorModel result;
        std::transform(begin(), end(), rhs.begin(), result.begin(), std::divides<>());
        return result;
    }
    
    ColorModel operator/(ColorModel&& rhs) const {
        ColorModel result;
        std::transform(begin(), end(), rhs.begin(), result.begin(), std::divides<>());
        return result;
    }
    
    ColorModel operator/(const T& rhs) const {
        ColorModel result;
#pragma clang loop vectorize(enable)
        for (int i = 0; i < size(); ++i) {
            result[i] = (*this)[i] / rhs;
        }
        return result;
    }
    
    ColorModel operator/(T&& rhs) const {
        ColorModel result;
#pragma clang loop vectorize(enable)
        for (int i = 0; i < size(); ++i) {
            result[i] = (*this)[i] / rhs;
        }
        return result;
    }
    
    
    bool operator==(const ColorModel& rhs) const {
        return std::equal(begin(), end(), rhs.begin(), rhs.end());
    }
    
    bool operator!=(const ColorModel& rhs) const {
        return !(*this == rhs);
    }
    
    
};

template<int Space, typename T, typename C>
std::ostream& operator<<(std::ostream& os, const ColorModel<Space, T, C>& space) {
    for (const T& component: space) {
        os << component << " ";
    }
    return os;
}

And my structs will be implemented like this:

struct RGB : ColorModel<0,unsigned char, RGB> {
    union {
        std::array<unsigned char,3> components;
        struct {
             unsigned char r,g,b;
        }
    }

    // Base constructors call 
};

I would be very grateful if you review this implementation of operator overloading.

P.S. The first argument int Space in the template argument list will be used after. I am going to write a logic to convert from one space to another, but that is a different question.

\$\endgroup\$

1 Answer 1

1
\$\begingroup\$

I'm not sure it's safe to union a standard array and a struct like that. I would make r, g and b be member functions instead:

unsigned char& r() { return components[0]; }
unsigned char const& r() const { return components[0]; }

(You could make r, g and b members be references into components, but compilers might reserve distinct space for them, and you wouldn't be able to use the default compiler-generated copy/move constructor and assignment).

Detailed review:

size_t size() const {

size_t is in the std namespace, and you need to include one of the header files that defines it.

#include <cstddef>
std::size_t size() const {

Looking at the implementation, it might be better to write size() in terms of the template parameter C. That would mean it could be declared constexpr, which could help compilers to optimise the arithmetic operators.

The arithmetic operators are all likely to overflow in use - you might want to implement some saturating arithmetic for the values to be useful in real image-processing.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you! May I ask you to review it again after I apply your suggestions? Or shall I ask it as a different question? \$\endgroup\$ Dec 3, 2020 at 19:27
  • \$\begingroup\$ It's generally best to ask as a new question. Two reasons: you can't expect me to still be available, and you really want more than one reviewer to give you the best range of suggestions. \$\endgroup\$ Dec 4, 2020 at 9:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.