What are the risks of variables with protected visibility

Question

I'm trying to implement a state pattern as explained in this example. I've gotten to code similar as below.

class State {
public:
    virtual void enter() {};
    virtual void update() = 0;
    virtual void exit() {};

    virtual void setContext(Context* cxt) {
        this->context = cxt;
    }
protected:
    Context* context;
};

class Context {
public:
    void do_something();
    void do_something_else();

    void transitionTo(std::unique_ptr<State> next_state) {
        if (state != nullptr) {
            state->exit();
        }
        state = std::move(next_state);
        state->setContext(this);
        state->enter();
    }

private:
    std::unique_ptr<State> state;
};

class ConcreteStateA : public State {
public:
    void update() override {
        try {
           context->do_something();
        } catch {
           context->transitionTo(std::unique_ptr<ConcreteStateB>());
        }
    }   
};

class ConcreteStateB {
 // ...
};

However, when i try to compile this with clang-tidy i get the following warning

error: member variable 'context' has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes,-warnings-as-errors]

I have the following 2 questions:

What are the risks on giving a variable with protected visibility?
Does anyone have any suggestions on how to solve this error in a clean way? (I've tought about creating a protected getter method, but if I want to act upon the correct context i will have to return a reference or pointer, which has the same effect as this, but just requires extra code).

I think the intended solution is to make it `private` and provide a `protected` getter. The risk highlighted may be that a derived type may assign to `context` which `State` doesn't seem intended to allow. I don't see why you would need to return a reference to the pointer, the point is to prevent someone making `context` point to a different object. `context` is a pointer to non-`const` so you can still act upon the pointed object. Edit : There is a `public` setter for the context, so I guess that reasoning doesn't really apply. But, that may still be the reason for the diagnostic. — François Andrieux, Jun 30 '21 at 15:29
It seems to me that `context->transitionTo(std::unique_ptr());` can just be `context->transitionTo(nullptr);`. In either case, the argument gets converted to `std::unique_ptr{nullptr}`. — François Andrieux, Jun 30 '21 at 15:34
The best thing to do is to go read the guideline associated: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-private In short, protected is better than public, but still offer the possibility to the subclasses to mess up any expectations you may have about the variable. — Julien Lopez, Jun 30 '21 at 15:58

score 2 · Accepted Answer · answered Jul 01 '21 at 09:49

You are not correct in saying

i will have to return a reference or pointer, which has the same effect as this, but just requires extra code

since exposing the pointer via making it protected allows the derived type to manipulate the pointer itself, not only the underlying data.
From documentation of the misc-non-private-member-variables-in-classes check for which cppcoreguidelines-non-private-member-variables-in-classes is an alias:

The data members should be declared as private and accessed through member functions instead of exposed to derived classes or class consumers.

score 0 · Answer 2 · answered Feb 01 '23 at 14:36

I would suggest that instead of calling member functions of Context that instead you create public (or protected) functions on State that manipulate the context. So:

class State {
public:
    virtual void enter() {};
    virtual void update() = 0;
    virtual void exit() {};

    virtual void setContext(Context* cxt) {
        this->context = cxt;
    }

protected:
    void do_something() { context->do_something(); }
    void transitionTo(std::unique_ptr<State> next_state) { context->transitionTo(std::move(next_state)); }

private:
    Context* context;
};

And then ConcreteStateA would be implemented as:

class ConcreteStateA : public State {
public:
    void update() override {
        try {
           do_something();
        } catch {
           transitionTo(std::unique_ptr<ConcreteStateB>());
        }
    }   
};

What are the risks of variables with protected visibility

2 Answers2