An implementation of the PIMPL idiom [closed]

Question

I would like to ask you to verify the following implementation of the PIMPL idiom. In this dummy implementation the stream is read into a string.

X.h

#ifndef X_H
#define X_H

#include <memory>
#include <istream>

namespace mynamespace
{
    class X
    {
        class XImplementation;
        std::unique_ptr<XImplementation> _implementation;
        X();
        void swap(X& other);
    public:
        X(std::istream& stream);
        X(const X& copyFrom);
        X& operator=(const X& copyFrom);
        X(X&& moveFrom);
        X& operator=(X&& moveFrom);
        ~X();
        std::string get_info();
    }
}
#endif // X_H

X.cpp

#include "X.h"

using namespace mynamespace;

class X::XImplementation
{
    std::string _contents;
public:
    XImplementation(std::istream& stream)
    : _contents(std::istreambuf_iterator<char>(stream), std::istreambuf_iterator<char>()) { }

    std::string get_info()
    {
        return _contents;
    }
};

X::X() = default;

void X::swap(X& other)
{
    std::swap(_implementation, other._implementation);
}

X::X(std::istream& stream) : _implementation(new X::XImplementation(stream)) { }

X::X(const X& copyFrom) : _implementation(new XImplementation(*copyFrom._implementation)) { };

X& X::operator=(const X& copyFrom)
{
    X temp(copyFrom);
    swap(temp);
    return *this;
}

X::X(X&& moveFrom)
{
    swap(moveFrom);
}

X& X::operator=(X&& moveFrom)
{
    swap(moveFrom);
    return *this;
}

X::~X() = default;

std::string X::get_info()
{
    return _implementation->get_info();
}

I tried my best to use copy-and-swap idiom to implement all the constructors. I wanted to make sure that X can be passed by value correctly. I am mostly wandering about this part, because I used a private method to implement the swap and here a public friend function is suggested.

I will appreciate any suggestions and critiques.

Answer 1

    X();

Since we specify a value constructor, the compiler won't generate the default constructor. So we don't need to declare it private to prevent its use.

---

The move operators can declared = default:

    X(X&& moveFrom) = default;
    X& operator=(X&& moveFrom) = default;

You actually appear to have two copy assignment operators instead of a move assignment operator!

---

std::swap(_implementation, other._implementation);

The best way to call swap is like so:

using std::swap;
swap(a, b);

This means that a swap function declared in the same namespace as the type being swapped will be found first. If there isn't one we'll fall back to using std::swap.

---

A public friend function is a common way to implement swap. Obviously a private function can't be called externally.

Note that we don't actually have to implement a swap function at all. The unspecialized version of std::swap will work for us, using our move constructor and move assignment operators.

---

namespace mynamespace ...

The class X is in this namespace, but the function implementations aren't?

---

Please check that your code compiles, runs, and behaves as desired before posting it for review!

Answer 2

An important point of the PIMPL is that it helps you hide details that you do not want to leak out to the public.

This means your header #include list is not expected to have more than the bare minimum and at times even no #include at all (although this last case is really rare).

Any class, struct, etc. used in your header file has to be declared in some way. For std::string, you can just do #include <string>. However, if you are implementing a class which access some Unix headers such as #include <sys/stat.h>, but your class definition doesn't need to present that part to the end user, then the PIMPL can be useful.

#include <sys/stat.h>

namespace mynamespace
{
class X::Implementation
{
    struct stat st = {};
...

Now your class has a stat object in its implementation, but from the outside you don't have direct access. Maybe you have functions such as is_dir() which return true if the file represents a directory. All of that without leaking the S_ISDIR() and similar macros offered by <sys/stat.h>.

If you do not have such #include in your .cpp file as a result, then your PIMPL is probably not useful. You can as well put all the fields you need in your main class (X in your case) and avoid having to duplicate all the functions.

That being said, in your case you have a swap() function which can make it useful to have a PIMPL just so you can easily implement that one function. I rarely see people doing that, but I think it's a valid case too.

---

As a side note, although you just don't need to declare the X() constructor, if you do not want people to use such, since C++11, we use the delete keyword like so:

class X
{
public:
    X() = delete;
    ...

If you use effective C++ warnings in g++ (i.e. g++ -Weffc++), then it is often required to delete the copy constructor and operators if (1) not necessary and (2) you have bare pointers in your class. The delete syntax is the best way to get rid of those two functions and as a result, the warnigs.