CODEWITHSUNDEEP
c++constants
Anne Quinn
Anne Quinn

Reputation: 13002

Is this usage of const_cast undefined in practice?

I have a class that manages input. To display and alter key bindings, it's important to give a caller a map of the bindings it can own and alter, before committing it to the manager when done. However, there are specific rules on what can be inserted/removed in this map that only the manager knows, thus the caller must be forced to ask the manager to make changes.

The caller gets a const version of the map to insure it cannot modify it on it's own, while the manager can still alter the map using a const_cast

typedef std::multimap<Key, Input> Map;

class InputManager{
    public:

    const Map getBindings(){
        // builds the map and returns it
    }

    bool insertBinding(const Map & bindings, Key key, Input input){
        // performs the insert after checking several rules first
        if(rulesAllowIt){
            const_cast<Map&>(bindings).insert(std::make_pair(key, input));
            return true;
        }else{
            return false;
        }
    }

    void commitBindings(const Map & bindings){
        // commits the bindings to replace the old
    }
}

This currently works as expected, but I'm worried by the usage of const_cast, since modifying a const variable is UB in principle (unless there are exceptions?)

In practice, will this cause UB, or any other subtle performance problems or bugs on different platforms or compilers, or conflict with possible optimizations?

Upvotes: 0

Views: 60

Answers (2)

Steve Jessop
Steve Jessop

Reputation: 279245

const Map getBindings() is a fairly pointless return type. The user can in any case write code this:

Map foo = manager.getBindings();
manager.insertBinding(foo, key, whatever);
foo.clear();
manager.commitBindings(foo);

In which case the code has defined behavior (at least, up to the point where commitBindings probably isn't expecting to see an empty map). Alternatively they could write this:

const Map foo = manager.getBindings();
manager.insertBinding(foo, key, whatever);
manager.commitBindings(foo);

In which case the code has UB per the standard. One way in which it could fail in practice, is that the optimizer is entitled to assume that since foo is const-qualified at the point where it is defined, then the values of its non-mutable data members will not change. Therefore, after inlining the code of commitBindings, it could re-order accesses to foo so that a data member is read before the call to insertBinding that results in it being modified.

In practice it doesn't seem like this is happening to your Map, but it's one reason why modifying const-qualified objects can become more than just a theoretical problem.

To get where you want to be, you could add another level of indirection (compile-time indirection: the compiler can remove the overheads). Define a class that has the Map as a private data member, and whose only public mutator has the same effect as insertBinding. Then the user has no means to modify the Map other than via the code that checks the rules. For efficiency, make sure that your class has a working move constructor and move assignment, since copying a std::multimap can be a lot of work.

Upvotes: 2

Sean
Sean

Reputation: 62472

This looks to me a bit like a code smell. Although it will work the reality is that you've declared your insertBindings method to take a constant map, and yet you're not honoring the const-ness within the method.

If you're going to potentially modify the map then just declare the type as non-const so that the method behaves in a way everyone would expect.

Upvotes: 2

Related Questions