Should setters of PIMPL classes be const member functions?

Question

I use "pointer to private implementation" classes often. The setter methods of those classes can technically be const member functions, such as this:

class MyPrivateClass
{
public:
  int something = 1;
};

class MyClass
{
public:

  // TODO: initialize pointer in constructor
  // TODO: delete pointer in destructor

  // Note how this setter is const!
  void setSomething(int something) const
    {
      p->something = something;
    }

private:
  MyPrivateClass* p;
};

int main()
{
  return 0;
}

This is possible since the compiler only enforces bitwise const, and not logical const, so the above code should compile just fine.

I think those setter methods should NOT be const member functions to let the callers know the object is actually being modified (logically modified, not bitwise modifed, due to the pointer to implementation).

My questions is:

Is there a good reason to make those setter methods const member functions?

Effective C++ advises (in item 3) to always use const whenever possible, but I don't think this applies to my example.

Cairns

I can't think why you would in almost all cases want a setter to be non-const, no matter how implemented. You might want to fiddle about with the const-ness of the implementation class. – Whomever

IMO it makes no sense for a setter to be const. If you actually did have a const object, you don't want setters to be called on it – Racket

Answer 1

"Use const whenever possible" is overly simplistic. It's a good starting point but not an absolute rule.

In the pimpl idiom, as you noted, applying the rule gives us const setters. But a strong counterargument is that this breaks encapsulation. Your interface is now reflecting an implementation choice. Imagine you refactored not to use pimpl. Users of your class shouldn't care about that completely internal decision, but now they do because you must remove const from the setters.

The same argument can be made any time there is private (to users) but remote (from the class) state. Refactoring to bring that state into the class will require logically non-const member functions not be marked const.

If you can imagine a reasonable implementation choice which would require the member function not be const, it is reasonable not to mark it as const.

There is a class template propagate_const in the library fundamentals TS, but which is easy to write by hand, which helps you with const-correctness in the pimpl idiom:

#include <experimental/propagate_const>
#include <memory>

template<class  T>
using pimpl = std::experimental::propagate_const<std::unique_ptr<T>>;

struct MyPrivateClass
{
    int something = 1;
};

struct  MyClass
{
    void setSomething(int something) const
    {
        // error: assignment of member 'MyPrivateClass::something' in read-only object
        p->something = something; 
    }

    pimpl<MyPrivateClass> p;
};

Also, note that in the other answer, the code sample does not compile:

error: decltype cannot resolve address of overloaded function
             is_const<decltype(&A::x)>::value == \
                                    ^
note: in expansion of macro 'GET'
     void f1()       { GET(f1).f1(); } // OK

Foreignborn

Thanks. I totally agree that "using const whenever possible" is overly simplistic. I just wanted to make sure I wasn't missing any good reason why using const on my example could be a good idea. – Cairns

Answer 2

In general, you want to mimic the qualifiers of the implementation class in the interface class.

If you want to get fancy, you are in C++11 and you use a lot the pimpl idiom, then you could ensure you are using the right qualifiers and you can also get an appropriately qualified reference to the implementation class doing something like:

#include <type_traits>

struct AImpl
{
    void f1();
    void f2() const;
};

template<typename T>
struct is_const;

template<typename R, typename T, typename... Args>
struct is_const<R (T::*)(Args...) const> : std::true_type {};

template<typename R, typename T, typename... Args>
struct is_const<R (T::*)(Args...)> : std::false_type {};

class A
{
    AImpl * p;

    template<class T>
    typename std::enable_if<!is_const<T>::value, AImpl &>::type get()
    {
        return *p;
    }

    template<class T>
    typename std::enable_if<is_const<T>::value, const AImpl &>::type get() const
    {
        return *p;
    }

public:
    #define GET(x) \
        static_assert( \
            is_const<decltype(&A::x)>::value == \
            is_const<decltype(&AImpl::x)>::value, \
            "Interface does not mimic the implementation" \
        ); \
        get<decltype(&AImpl::x)>()

    void f1()       { GET(f1).f1(); } // OK
    void f1() const { GET(f1).f1(); } // Error

    void f2()       { GET(f2).f2(); } // Error
    void f2() const { GET(f2).f2(); } // OK

    #undef GET
};

get<T>() returns a const reference to the implementation if the member function pointer T is const; otherwise, a non-const one. Using this would already cover one error case and gives you an appropriately qualified reference.

If you want to push a little further, GET() also checks that the constness of the interface is the same as the implementation, covering the other case.

Toll