Overloading bit operators for scoped enum

Question

The code snippet is used to record the status of several cameras.

Since the enum class MODULE_ENABLED should not be used by the customers directly, I marked it as private. And I overload several bit operators for the said scoped enumeration.

Since the enum class MODULE_ENABLED is marked as private, the bit overloading operators for this type can't be invoked by the customers.

How to improve the code snippet below? Is there anything I missed?

#include <iostream>
#include <stdint.h>
#include <type_traits>

class ModuleSwitch
{
private:
    enum class MODULE_ENABLED : uint8_t
    {
        MODULE_ENABLED_NONE = 0,
        MODULE_ENABLED_FRONT_CAMERA = 1,
        MODULE_ENABLED_MIDDLE_CAMERA = 2,
        MODULE_ENABLED_BACK_CAMERA = 4,
    };
public:
    ModuleSwitch();
    bool is_front_camera_enabled() const;
    bool is_middle_camera_enabled() const;
    bool is_back_camera_enabled() const;
    void set_front_camera_enb(bool flag);
    void set_middle_camera_enb(bool flag);
    void set_back_camera_enb(bool flag);
private:
    void set_mod_enb(bool flag, const MODULE_ENABLED& a);
    friend MODULE_ENABLED operator|(const MODULE_ENABLED& a, const MODULE_ENABLED& b);
    friend MODULE_ENABLED& operator|=(MODULE_ENABLED& a, const MODULE_ENABLED& b);
    friend MODULE_ENABLED operator&(const MODULE_ENABLED& a, const MODULE_ENABLED& b);
    friend MODULE_ENABLED operator&=(MODULE_ENABLED& a, const MODULE_ENABLED& b);
    friend MODULE_ENABLED operator^(const MODULE_ENABLED& a, const MODULE_ENABLED& b);
    friend MODULE_ENABLED& operator^=(MODULE_ENABLED& a, const MODULE_ENABLED& b);
    friend MODULE_ENABLED operator~(const MODULE_ENABLED& a);
private:
    MODULE_ENABLED m_mod_status;
};

ModuleSwitch::ModuleSwitch(): m_mod_status(MODULE_ENABLED::MODULE_ENABLED_NONE){};

ModuleSwitch::MODULE_ENABLED operator|(const ModuleSwitch::MODULE_ENABLED& a, const ModuleSwitch::MODULE_ENABLED& b)
{
    using MODULE_ENABLED = ModuleSwitch::MODULE_ENABLED;
    
    return static_cast<MODULE_ENABLED>(static_cast<std::underlying_type<MODULE_ENABLED>::type>(a) | static_cast<std::underlying_type<MODULE_ENABLED>::type>(b));
}

ModuleSwitch::MODULE_ENABLED& operator|=(ModuleSwitch::MODULE_ENABLED& a, const ModuleSwitch::MODULE_ENABLED& b)
{
    a=(a|b);
    return a;
}

ModuleSwitch::MODULE_ENABLED operator&(const ModuleSwitch::MODULE_ENABLED& a, const ModuleSwitch::MODULE_ENABLED& b)
{
    using MODULE_ENABLED = ModuleSwitch::MODULE_ENABLED;

    return static_cast<MODULE_ENABLED>(static_cast<std::underlying_type<MODULE_ENABLED>::type>(a) & static_cast<std::underlying_type<MODULE_ENABLED>::type>(b));
}

ModuleSwitch::MODULE_ENABLED operator&=(ModuleSwitch::MODULE_ENABLED& a, const ModuleSwitch::MODULE_ENABLED& b)
{
    a = a&b;
    return a;
}

ModuleSwitch::MODULE_ENABLED operator^(const ModuleSwitch::MODULE_ENABLED& a, const ModuleSwitch::MODULE_ENABLED& b)
{
    using MODULE_ENABLED = ModuleSwitch::MODULE_ENABLED;

    return static_cast<MODULE_ENABLED>(static_cast<std::underlying_type<MODULE_ENABLED>::type>(a) ^ static_cast<std::underlying_type<MODULE_ENABLED>::type>(b));
}

ModuleSwitch::MODULE_ENABLED& operator^=(ModuleSwitch::MODULE_ENABLED& a, const ModuleSwitch::MODULE_ENABLED& b)
{
    a = a ^ b;
    return a;
}

ModuleSwitch::MODULE_ENABLED operator~(const ModuleSwitch::MODULE_ENABLED& a)
{
    using MODULE_ENABLED = ModuleSwitch::MODULE_ENABLED;
    return static_cast<MODULE_ENABLED>(~static_cast<std::underlying_type<MODULE_ENABLED>::type>(a));
}

bool ModuleSwitch::is_front_camera_enabled() const
{
    return (m_mod_status & MODULE_ENABLED::MODULE_ENABLED_FRONT_CAMERA) != MODULE_ENABLED::MODULE_ENABLED_NONE;
}

bool ModuleSwitch::is_middle_camera_enabled() const
{
    return (m_mod_status & MODULE_ENABLED::MODULE_ENABLED_MIDDLE_CAMERA) != MODULE_ENABLED::MODULE_ENABLED_NONE;
}

bool ModuleSwitch::is_back_camera_enabled() const
{
    return (m_mod_status & MODULE_ENABLED::MODULE_ENABLED_BACK_CAMERA) != MODULE_ENABLED::MODULE_ENABLED_NONE;
}

void ModuleSwitch::set_mod_enb(bool flag, const MODULE_ENABLED& mod_enb)
{
    if(flag)
    {
        m_mod_status |= mod_enb;
    }
    else
    {
        m_mod_status &= ~mod_enb;
    }
}

void ModuleSwitch::set_front_camera_enb(bool flag)
{
    set_mod_enb(flag, MODULE_ENABLED::MODULE_ENABLED_FRONT_CAMERA);
}
void ModuleSwitch::set_middle_camera_enb(bool flag)
{
    set_mod_enb(flag, MODULE_ENABLED::MODULE_ENABLED_MIDDLE_CAMERA);
}

void ModuleSwitch::set_back_camera_enb(bool flag)
{
    set_mod_enb(flag, MODULE_ENABLED::MODULE_ENABLED_BACK_CAMERA);
}

int main() {
    ModuleSwitch mod_swt;
    auto print_camera_status = [](const ModuleSwitch& mod_swt){
        std::cout << "front camera enabled:" << mod_swt.is_front_camera_enabled() << std::endl;
        std::cout << "middle camera enabled:" << mod_swt.is_middle_camera_enabled() << std::endl;;
        std::cout << "back camera enabled:" << mod_swt.is_back_camera_enabled() << std::endl;;
    };

    std::cout <<"#1" << std::endl;
    mod_swt.set_front_camera_enb(true);
    print_camera_status(mod_swt);

    std::cout <<"#2" << std::endl;
    mod_swt.set_middle_camera_enb(true);
    print_camera_status(mod_swt);

    std::cout <<"#3" << std::endl;
    mod_swt.set_middle_camera_enb(false);
    print_camera_status(mod_swt);
    
    return 0;
}

G. Sliepen · Accepted Answer · 2024-05-16 16:46:25Z

Naming things

Since MODULE_ENABLED is a scoped enum, the names of the enum values are not in the global namespace, so you don't need to prefix them with MODULE_ENABLED_.

Avoid using ALL_CAPS for anything but macros. MODULE_ENABLED is not a macro but a type, so use the same naming scheme as you use for other types. In your case, that would be ModuleEnabled.

Even then, ModuleEnabled is a bit of a weird name for a type, because whether something or not is enabled is actually determined by a concrete value. Also, since this is about camera positions specifically, I would rename this to CameraPosition. There is also no need to repeat _CAMERA in the names of the enum values. So:

enum class CameraPosition : uint8_t {
    none = 0,
    front = 1,
    middle = 2,
    back = 4,
};

Don't abbreviate names unnecessarily. Instead of set_mod_enb(), just write set_module_enabled(). But again, don't repeat things unnecessarily in the names of class members: since it's already about modules, set_enabled() is enough.

Consider exposing `enum class CameraPosition`

What if you add left and right cameras. The problem is then that you have to add is_left_camera_enabled(), is_right_camera_enabled(), set_left_camera_enabled() and so on. And if you remove a camera, you have to remove multiple member functions. Why not just make CameraPosition public, as well as set_enabled(), so the caller can pass in an enum value. You can then also add an is_enabled() member function. Alternatively, you can split set_enabled() into an enable() and disable() function:

class ModuleSwitch {
public:
    enum class CameraPosition: uint8_t {
        none = 0,
        front = 1,
        middle = 2,
        back = 4,
    };

    void enable(CameraPosition position);
    void disable(CameraPosition position);
    bool is_enabled(CameraPosition position);

private:
    friend CameraPosition operator|(CameraPosition a, CameraPosition b);
    …
    CameraPosition m_enabled_positions;
};

void ModeSwitch::enable(ModeSwitch::CameraPosition position) {
    m_enabled_positions |= position;
}

void ModeSwitch::disable(ModeSwitch::CameraPosition position) {
    m_enabled_positions &= ~position;
}

bool ModeSwitch::is_enabled(ModeSwitch::CameraPosition position) {
    return m_enabled_positions & position == position;
}

Now your main() will look like:

int main() {
    ModuleSwitch module_switch;
    auto print_camera_status = [](const ModuleSwitch& module_switch){
        std::cout << "front camera enabled:" << module_switch.is_enabled(ModuleSwitch::CameraPosition::front) << '\n';
        …
    };

    std::cout << "#1\n";
    module_switch.enable(ModuleSwitch::CameraPosition::front);
    print_camera_status(module_switch);
    …
}

There's a slight increase in how much you have to type, but class ModuleSwitch is now much smaller, and easier to maintain and modify. Also, since C++20 you can use using enum which can greatly reduce how often you have to repeat the enum type's name.

Make it more generic

The friend functions you created to operate on CameraPosition values are fine. However, they only work for CameraPosition. What if you need this kind of functionality for another enum class as well? Ideally, you would create generic functions once that work for any enum type you desire. It is possible with some work. Have a look at this and this code review for example, or this software engineering question.

Note that you can even make the functionality enable(), disable() and is_enabled() generic, and then you wouldn't need a class ModeSwitch anymore, just the enum class CameraPosition is enough, so your code would look like:

// No longer inside another class
enum class CameraPosition : uint8_t {
    …
};

// Code to enable bitfield operations on CameraPosition goes here
…

int main() {
    CameraPosition module_switch;
    auto print_camera_status = [](const CameraPosition& module_switch){
        std::cout << "front camera enabled:" << is_enabled(module_switch, CameraPosition::front) << '\n';
        …
    };

    std::cout << "#1\n";
    enable(module_switch, CameraPosition::front);
    print_camera_status(module_switch);
    …
}

Thank you for the detailed guidiance. I still have a questions. You say " no need to prefix them with MODULE_ENABLED_", but I really can't see any MODULE_ENABLED_ indeed. Am I misunderstand anything? — John
– John, Commented May 17, 2024 at 2:41
I meant remove MODULE_ENABLED_ from names like MODULE_ENABLED_FRONT_CAMERA, so only FRONT_CAMERA is left. — G. Sliepen
– G. Sliepen, Commented May 17, 2024 at 7:22
Thank you so much. I modified the code snippet to fllow your advice. For creating generic overloading bitwise functions to manipulate the enum type, I think my implemetation can acomplish this goal indeed. The links you aforementioned in your answer use a little complex way to achieve the said goal which is still hard for me to read and understand(I will try my best to fully understand them later:) ). Too long, see the next comment, please. — John
– John, Commented May 21, 2024 at 8:27
I have created a new review to find out is there any potential problem with my implenmentation. — John
– John, Commented May 21, 2024 at 8:27

Stack Exchange Network

Overloading bit operators for scoped enum

1 Answer 1

Naming things

Consider exposing `enum class CameraPosition`

Make it more generic

You must log in to answer this question.

Linked

Hot Network Questions

Overloading bit operators for scoped enum

1 Answer 1

Naming things

Consider exposing enum class CameraPosition

Make it more generic

You must log in to answer this question.

Linked

Related

Hot Network Questions

Consider exposing `enum class CameraPosition`