3
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ mynamespace::X doesn't look like real code from a project you wrote or maintain, so isn't within scope for Code Review. See help centre. \$\endgroup\$ Jul 26, 2021 at 15:43
  • 1
    \$\begingroup\$ Does not really implement the PIMP pattern, does it. You are just asking if you have correctly implemented the Copy/Move semantics of an object with an owned pointer. \$\endgroup\$ Jul 26, 2021 at 17:59
  • \$\begingroup\$ @TobySpeight yes, you are correct, this is not the real code. I apologize, I didn't know this rule and I will need to accept if you decide to close/downvote this question. I will try to take care about that rule in the future. I would like to say that the accepted answer has helped me and I am grateful for it. \$\endgroup\$ Jul 26, 2021 at 18:45
  • \$\begingroup\$ @MartinYork could you elaborate what would be a PIMPL pattern? I have to admit that I had biggest issues with the copy/move semantics, but I thought that forwarding the method call to the implementation is the PIMPL pattern as demonstrated by the get_info method. Am I wrong? \$\endgroup\$ Jul 26, 2021 at 18:50
  • 2
    \$\begingroup\$ bfilipek.com/2018/01/pimpl.html The pimple pattern uses one class to implement the interface and another class that holds an object to that implementation and forwards any calls. But both of these classes have the same interface. Are you saying that your interface is get_info() sure. \$\endgroup\$ Jul 26, 2021 at 21:28

2 Answers 2

6
\$\begingroup\$
    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!

\$\endgroup\$
1
  • \$\begingroup\$ I would like to apologize for not compiling the code, I have corrected and try to highlight the differences. Thank you for the response. \$\endgroup\$ Jul 26, 2021 at 12:27
2
\$\begingroup\$

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.

\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.