Should I declare these methods const?

问题:

I'm working on some C++ code where I have several manager objects with private methods such as

void NotifyFooUpdated();

which call the OnFooUpdated() method on the listeners of this object.

Note that they don't modify the state of this object, so they could technically be made const methods, even though they typically modify the state of the system as a whole. In particular, the listener objects might call back into this object and modify it.

Personally I'd like to leave them as they are and not declare them const.

However, our static code checker QAC flags this as a deviation, so I either have to declare them const, or I have to argue why they should stay non-const and get a grant for the deviation.

What are arguments for not declaring these methods const?
Or should I follow QAC and declare them const?
Should I adopt a strictly local viewpoint restricted to this object, or consider the system as a whole?

回答1:

Loosely speaking you have a container class: A manager full of observers. In C and C++ you can have const containers with non-const values. Consider if you removed one layer of wrapping:

list<Observer> someManager;

void NotifyFooUpdated(const list<Observer>& manager) { ... }

You would see nothing strange about a global NotifyFooUpdated taking a const list, since it does not modify the list. That const argument actually makes the argument parsing more permissive: The function accepts both const and non-const lists. All the const annotation on the class method version means is const *this.

To address another perspective:

If you can't guarantee that the object you invoked the function on remains the same before and after the function call, you should generally leave that as non-const.

That's only reasonable if the caller has the only reference to the object. If the object is global (as it is in the original question) or in a threaded environment, the constness of any given call does not guarantee the state of the object is unchanged across the call. A function with no side-effects and which always returns the same value for the same inputs is pure. NotifyFooUpdate() is clearly not pure.

回答2:

If the listeners are stored as a collection of pointers you can call a non-const method on them even if your object is const.

If the contract is that a listener may update its state when it gets a notification, then the method should be non-const.

You are saying that the listener may call back into the object and modify it. But the listener will not change itself - so the Notify call could be const but you pass a non-const pointer to your own object into it.

If the listener already has that pointer (it listens only to one thing) then you can make both the methods const, as your object getting modified is a side-effect. What is happening is:

A calls B B modifies A as a result.

So A calling B leads indirectly to its own modification but is not a direct modification of self.

If this is the case both your methods could and probably should be const.

回答3:

What are arguments for not declaring these methods const?
Or should I follow QAC and declare them const?
Should I adopt a strictly local viewpoint restricted to this object, or consider the system as a whole?

What you know is that that manager object this was called for do not change. The objects the manager then invokes functions on might change or they might not. You don't know that.

From your description I could envision such a design where all involved objects are const (and notifications might be processed by writing them to the console). If you don't make this function const, you prohibit this. If you make it const, you allow both.

I guess this is an argument in favor of making this const.

回答4:

If you can't guarantee that the object you invoked the function on remains the same before and after the function call, you should generally leave that as non-const. Think about it - you could write a listener, insert it while the object is non-const, and then use this function to violate const correctness because you once had access to that object when it was non-const in the past. That's wrong.

回答5:

My take on it is that they should remain non const. This is based on my perception that the manager object's state, is actually the aggregate of the states of all the objects it manages plus any intrinsic state i.e., State(Manager) = State(Listener0) + State(Listener1) + ... + State(ListenerN) + IntrinsicState(Manager).

While encapsulation in the source code may belie this run-time relationship. Based on your description, I believe this aggregated state is reflective of the run-time behavior of the program.

To reinforce my argument: I assert that the code should strive to reflect the programs run-time behavior in preference to strict adherence to the exact semantics of compilation.

回答6:

To const, or not to const: that is the question.

Arguments for const:

• The methods in question don't modify the state of the object.
• Your static code checker flags the lack of const as a deviation, maybe you should listen to it.

Arguments against const:

• The methods modify the state of the system as a whole.
• Listener objects my modify the object.

Personally I'd go with leaving it as const, the fact that it might modify the state of the system as a whole is pretty much like a null pointer reference. It's a const method, it doesn't modify the object in question, but it will crash your program thereby modifying the state of the whole system.

回答7:

There are some good arguments for against const, here so here's my take :-

Personally, I'd not have these "OnXXXUpdated" as part of my manager classes. I think this is why there is some confusion as to best-practice. You're notifying interested parties about something, and do not know whether or not the state of the object is going to change during the notification process. It may, or may not. What is obvious to me, is that the process of notifying interested parties should be a const.

So, to solve this dilemma, this is what I would do:

Get rid of the OnXXXXUpdated functions from your manager classes.

Write a Notification Manager, here's a prototype, with the following assumptions:

"Args" is an arbitrary base-class for passing information when notifications happen

"Delegate" is some kind of a function pointer (e.g FastDelegate).

class Args
{
};

class NotificationManager
{
private:
    class NotifyEntry
    {
    private:
        std::list<Delegate> m_Delegates;

    public:
        NotifyEntry(){};
        void raise(const Args& _args) const
        {
            for(std::list<Delegate>::const_iterator cit(m_Delegates.begin());
                cit != m_Delegates.end();
                ++cit)
                (*cit)(_args);
        };

        NotifyEntry& operator += (Delegate _delegate) {m_Delegates.push_back(_delegate); return(*this); };
    }; // eo class NotifyEntry

    std::map<std::string, NotifyEntry*> m_Entries;

public:
    // ctor, dtor, etc....

    // methods
    void register(const std::string& _name);     // register a notification ...
    void unRegister(const std::string& _name);   // unregister it ...

    // Notify interested parties
    void notify(const std::string& _name, const Args& _args) const
    {
        std::map<std::string, NotifyEntry*>::const_iterator cit = m_Entries.find(_name);
        if(cit != m_Entries.end())
           cit.second->raise(_args);
    }; // eo notify

    // Tell the manager we're interested in an event
    void listenFor(const std::string& _name, Delegate _delegate)
    {
        std::map<std::string, NotifyEntry*>::const_iterator cit = m_Entries.find(_name);
        if(cit != m_Entries.end())
            (*cit.second) += _delegate;
    }; // eo listenFor
}; // eo class NotifyManager

I've left some code out as you can probably tell, but you get the idea. I imagine that this Notification Manager would be a singleton. Now, ensuring that the Notification Manager is created early on, the rest of your managers simply register their notifications in their constructor like this:

MyManager::MyManager()
{
    NotificationMananger.getSingleton().register("OnABCUpdated");
    NotificationMananger.getSingleton().register("OnXYZUpdated");
};


AnotherManager::AnotherManager()
{
    NotificationManager.getSingleton().register("TheFoxIsInTheHenHouse");
};

Now, when your manager needs to notify interested parties, it simply calls notify:

MyManager::someFunction()
{
    CustomArgs args; // custom arguments derived from Args
    NotificationManager::getSingleton().notify("OnABCUpdated", args);
};

Other classes can listen for this stuff.

I've realised that I've just typed out the Observer pattern, but my intention was to show that the problem is in how these things are being raised and whether they are in a const-state or not. By abstracted the process of notification out of the mananager class, recipients of the notification are free to modify that manager class. Just not the notification manager. I think this is fair.

Besides, having a single place to raise notifications is good pracice imho, as it gives you a single place where you can trace your notifications.

回答8:

I guess you are following HICPP or something similar.

What we do is if our code violates QACPP and we think it is in error then we note it via Doxygen (via addtogroup command so you get a list of them easily), give a reason jusifying why we are violating it and then disable the warning via the //PRQA command.

回答9:

Note that they don't modify the state of this object, so they could technically be made const methods, even though they typically modify the state of the system as a whole. In particular, the listener objects might call back into this object and modify it.

Since the listener can change a state, then this method should not be const. From what you wrote, it sounds like you are using lots of const_cast and call through pointers.

回答10:

const correctness has a (intentionally desirable) way of propagating. you should use const wherever you can get away with it, while const_cast and c-style-casts should be artifacts of dealing with client code - never in your code, but very very rare exceptions.

if void NotifyFooUpdated(); calls listeners[all].OnFooUpdated() while on OnFooUpdated() is not const, then you should explicitly qualify this mutation. if your code is const correct throughout (which i am questioning), then make it explicit (via method declaration/listener access) that you are mutating the listeners (members), then NotifyFooUpdated() should qualify as non-const. so, you just declare the mutation as close to the source as possible and it should check out and const-correctness will propagate properly.

回答11:

Making virtual functions const is always a difficult decision. Making them non-const is the easy way out. A listener function should be const in many cases: if it doesn't change the listening aspect (for this object). If listening to an event would cause the listening party to unregister itself (as a general case), then this function should be non-const.

Although the internal state of the object may change on a OnFooChanged call, at the level of the interface, the next time OnFooChanged is called, will have a similar result. Which makes it const.

回答12:

When using const in your classes you're helping users of that class know how the class will interact with data. You're making a contract. When you have a reference to a const object, you know that any calls made on that object won't change its state. The constness of that reference is still only a contract with the caller. The object is still free to do some non-const actions in the background using mutable variables. This is particularly useful when caching information.

For example you can have class with the method:

int expensiveOperation() const
{
    if (!mPerformedFetch)
    {
        mValueCache = fetchExpensiveValue();
        mPerformedFetch = true;
    }
    return mValueCache;
}

This method may take a long time to perform the first time, but will cache the result for subsequent calls. You only need to make sure the header file declares the variables performedFetch and valueCache as mutable.

class X
{
public:
    int expensiveOperation() const;
private:
    int fetchExpensiveValue() const;

    mutable bool mPerformedFetch;
    mutable int mValueCache;
};

This lets a const object make the contract with the caller, and act like it is const while working a little smarter in the background.

I would suggest making your class declare the list of listeners as mutable, and make everything else as const as possible. As far as the object's caller is concerned, the object is still const and acting that way.

回答13:

Const means the state of the object is not modified by the member function, no more, no less. It has nothing to do with side effects. So if I understand your case correctly, the object's state doesn't change that means the function must be declared const, the state of the other parts of your application has nothing to do with this object. Even when sometimes the object state has non-const sub-objects, which are not a part of the object's logical state (e.g. mutexes), the functions still have to be made const, and those parts must be declared mutable.