C++ PixelComponent struct

Question

Hey I wrote a struct in C++ trying to represent a Pixel Component of a color Here it is

template<typename T, T min, T max>
struct PixelComponent {
    static constexpr T MIN = min;
    static constexpr T MAX = max;
    T value;

    PixelComponent() = default;

    PixelComponent(T value) : value(value) {}

    inline bool operator==(const PixelComponent &other) {
        return value == other.value;
    }

    inline bool operator!=(const PixelComponent &other) {
        return !(*this == other);
    }

    inline bool operator>(const PixelComponent &other) {
        return value > other.value;
    }

    inline bool operator<(const PixelComponent &other) {
        return !(*this > other) && *this != other;
    }

    inline bool operator>=(const PixelComponent& other){
        return !(*this < other);
    }

    inline bool operator<=(const PixelComponent& other){
        return !(*this > other);
    }

    inline PixelComponent &operator=(const T &elem) {
        if (value == elem) {
            return *this;
        }
        value = elem > MAX ? MAX : elem < MIN ? MIN : elem;
        return *this;
    }

    inline PixelComponent &operator=(const PixelComponent &other) {
        if (*this == other) {
            return *this;
        }
        *this = other.value;
        return *this;
    }

    inline PixelComponent &operator++(){
        *this = ++value;
        return *this;
    }

    inline PixelComponent &operator--(){
        *this = --value;
        return *this;
    }

    inline PixelComponent operator+(const T& elem){
        PixelComponent result;
        result = value + elem;
        return result;
    }

    inline PixelComponent operator-(const T& elem){
        PixelComponent result;
        result = value - elem;
        return result;
    }

    inline PixelComponent operator*(const T& elem){
        PixelComponent result;
        result = value * elem;
        return result;
    }

    inline PixelComponent operator/(const T& elem){
        PixelComponent result;
        result = value / elem;
        return result;
    }

    inline PixelComponent operator+(const PixelComponent& other){
        PixelComponent result;
        result = value + other.value;
        return result;
    }

    inline PixelComponent operator-(const PixelComponent& other){
        PixelComponent result;
        result = value - other.value;
        return result;
    }

    inline PixelComponent operator*(const PixelComponent& other){
        PixelComponent result;
        result = value * other.value;
        return result;
    }

    inline PixelComponent operator/(const PixelComponent& other){
        PixelComponent result;
        result = value / other.value;
        return result;
    }

    inline PixelComponent operator+=(const T& elem){
        *this = *this + elem;
        return *this;
    }

    inline PixelComponent operator-=(const T& elem){
        *this = *this - elem;
        return *this;
    }

    inline PixelComponent operator*=(const T& elem){
        *this = *this * elem;
        return *this;
    }

    inline PixelComponent operator/=(const T& elem){
        *this = *this / elem;
        return *this;
    }

    inline PixelComponent& operator+=(const PixelComponent& other){
        *this = *this + other;
        return *this;
    }

    inline PixelComponent& operator-=(const PixelComponent& other){
        *this = *this - other;
        return *this;
    }

    inline PixelComponent& operator*=(const PixelComponent& other){
        *this = *this * other;
        return *this;
    }

    inline PixelComponent& operator/=(const PixelComponent& other){
        *this = *this / other;
        return *this;
    }


};

template<typename T, T min, T max>
std::ostream &operator<<(std::ostream &os, const PixelComponent<T, min, max> &component) {
    os << component.value;
    return os;
}

Please review this. I will be thankful for any suggestion or typos noticed) Please also review implementation of value always staying in range [MIN,MAX]. I have not added that validation in constructor, is there a nice way to do that?

Answer 1

Why?

I don't see any benefit to using this PixelComponent class over simply using the value type directly, because...

---

Clamping

... clamping to min and max values is less helpful than it appears.

We might want a color to be clamped to the range 0.f to 1.f in our output. However, it's very useful to do calculations with colors outside of that range as an intermediate step (e.g. (a + b) * 0.5f). Clamping prevents us from doing that.

Alternatively, if we were using a std::uint8_t as a color component (range 0 to 255), clamping the value would be meaningless as we're already using the full range of the type (and misleading, since we'd overflow before the clamping occurred).

So ultimately I'd suggest doing the clamping explicitly, when we need to as part of our color manipulation algorithm, and not in the pixel type itself.

Note also that we can use std::clamp(), rather than writing our own clamping function.

---

Operator overloads

inline PixelComponent operator+(const PixelComponent& other){
    PixelComponent result;
    result = value + other.value;
    return result;
}

inline PixelComponent operator+=(const T& elem){
    *this = *this + elem;
    return *this;
}

The inline keyword isn't necessary if we put the function definition inside the class.

operator+= (and the other math-assignment operators) should return a PixelComponent&, not a copy.

For a simple POD type, we can pass the parameter by value instead of by reference.

We'd normally implement operator+= to make the actual changes to the value in-place. And then implement operator+ using operator+=. This saves us from making unnecessary copies (although that isn't a big deal with a single POD value).

---

inline bool operator<(const PixelComponent &other) {
    return !(*this > other) && *this != other;
}

Could be written as return (other > *this);

---

Missing functions:

inline PixelComponent &operator++() ...

Don't forget to overload the post-increment operator as well: PixelComponent operator++(int)

We're missing modular arithmetic operators: a %= b a % b

We're missing the unary negation (and plus) operators: -a +a

It's reasonable to want to do bitwise operations on integer color values, so maybe we should have those too...

Answer 2

Not sure if you really need an own class definition. Also your code doesn't do anything apart from the automatic clamping.

I would suggest using something like

using PixelT = uint8_t;

somewhere in your code. Maybe it would also be interesting to see an example of how your code is used to suggest alternatives. One alternative would surely be to use the std::clamp function. I guess you wouldn't have too many places in your code where you actually would add or multiply pixels. Using std::clamp here - or even your own version of clamp with fixed min/max values would probably work as well.

Just a few hints about "style":

1. Prefer direct initialization - or direct returns - over declaring a variable, then initializing it - e.g. writing

   PixelComponent operator/(const T& elem){
     return PixelComponent(value / elem);
   }
   

   Of course for this to work in your example the constructor that takes a value should also clamp the value - not only the assignment operator. Otherwise your interface will stick to this awkward need to invoke the operator=.

2. Do not use inline when not necessary - this should only be necessary for definitions outside of the class body inside the header. Nowadays the compiler would optimize automatically and would not consider the inline keyword any more.

3. Try to define explicit constructors (when one argument is given) - this inhibits conversion to a different type when that was actually not intended.

See many many other rules in the Cpp Core Guidelines - yes - this language is complicated. But have fun anyway!