Is there any reason that the following pimpl-like implementation wouldn't work?

Question

Here I asked a question about trying to avoid polluting my codebase with content from the window.h file so as to make sure my codebase is cross-platform compatible. I was shown that the general idea that I'd come up with with referred to the pimpl idiom. Today I took the idiom a step forward or a step backward and I'm not certain which. I'm fairly certain the implementation that I have won't technically cause problems, but is a bit ugly. My main question is this: is anybody aware of any reason that the implementation will cause problems? Also are there any methods to make it less... well... ugly and more easily maintained?

The file ElementsToHide.h simulates contents of a file that I don't want polluting the global namespace across my project, such as windows.h.

The file ClassWithPImpl.h contains a definition of a PImpl struct that just contains a char[16], but only defines the struct if a flag hasn't been defined to indicate it's previous declaration. The char[16] is just there to keep the size of the struct the same throughout all of the files.

The file ClassWithPImpl.cpp of course defines the class's functionality, but unlike any other files that might include ClassWithPImpl.h, ClassWithPImpl.cpp defines it's own version of the PImpl struct, which uses datatypes from ElementsToHide.h. The size of these two structs are the same, which means that when ClassWithPImpl.cpp compiles it is able to see the actual contents of the PImpl struct. Every other file just sees a char array which is private. This means that none of the other files includes ElementsToHide.h and thus none of the other files are polluted with it's datatypes, functions, etc...

main.cpp is a tiny short program that prints out the sizes of the classes and PImpl structs to make sure they're the same. A commercial application would probably use asserts to make sure the structs stay the same size.

Here are all of the files in the application: ElementsToHide.h

#ifndef ELEMENTS_TO_HIDE_H
#define ELEMENTS_TO_HIDE_H

typedef short int16;
typedef long int32;
typedef long long int64;

#endif

ClassWithPImpl.h

#ifndef CLASS_WITH_PIMPL_H
#define CLASS_WITH_PIMPL_H

#ifndef PIMPL_DEFINED
#define PIMPL_DEFINED
struct PImpl
{
    char data[16];
};
#else
struct PImpl;
#endif

class ClassWithPImpl
{
private:
    PImpl pImpl;
public:
    ClassWithPImpl();
    void print();
};

#endif

ClassWithPImpl.cpp

#include <iostream>
using namespace std;

#include "ElementsToHide.h"

#define PIMPL_DEFINED
struct PImpl
{
    int16 shortInt;
    int32 mediumInt;
    int64 longInt;
};

#include "ClassWithPImpl.h"

ClassWithPImpl::ClassWithPImpl()
{
    pImpl.shortInt = 4;
    pImpl.mediumInt = 1;
    pImpl.longInt = 2;
}

void ClassWithPImpl::print()
{
    cout << pImpl.shortInt << endl;
    cout << "Size of class as seen from class: " << sizeof(ClassWithPImpl) << endl;
    cout << "Size of PImpl as seen from class: " << sizeof(PImpl) << endl;
}

main.cpp

#include <iostream>
using namespace std;

#include "ClassWithPImpl.h"

int main()
{
    //int32 shortInt;
    //Program won't compile with this line, because despite the ClassWithPImpl using the int32 type defined in ElementsToHide.h, these types are never
    //actually included into main.cpp.
    ClassWithPImpl().print();
    cout << "Size of Class as seen from Main: " << sizeof(ClassWithPImpl) << endl;
    cout << "Size of PImple as seen from Main: " << sizeof(PImpl) << endl;
    cout << sizeof(short) << ", " << sizeof(long) << ", " << sizeof(long long) << endl;
}

The output of the application is:

4
Size of class as seen from class: 16
Size of PImpl as seen from class: 16
Size of Class as seen from Main: 16
Size of PImple as seen from Main: 16
2, 4, 8

As you can see, the class file can see and deal with the internal variables fine, while the rest of the application is truly oblivious that the application has any internal variables other than a char[16]. The default copy constructor/operator should work just file because it'll just copy over the array contents. There's no need for a destructor because there's no managed resources in the class. There's also no wasted CPU time referencing a pointer to the hidden data structure, it's actually contained directly within the class definition when viewed from the implementation file. It is ugly. Is anybody aware of situations that this wouldn't work or that would complicate it even further? Are there any ways to simplify it without resorting to the pointer which has to be managed and dereferenced any time you want to get data from it?

Answer 1

The pointer-to-implementation idiom uses... wait for it... a pointer! So, the data that would have been private is dynamically allocated with new. The code you've posted uses an opaque buffer that you're hoping will remain enough while not being overly wasteful, which compromises some of the benefits of pImpl but saves on dynamic allocation and deallocation time when it does work. So, best not to refer to this as pImpl.

My main question is this: is anybody aware of any reason that the implementation will cause problems?

The #ifndef changing the content of your data-hiding-class across translation units gives Undefined Behaviour per 3.2/6:

each definition of D shall consist of the same sequence of tokens;

So, there's no guarantee it will work. If you're feeling lucky, you may still want to check that the Standard guarantees to put your class objects on a suitably aligned boundary - even when the "content" is ostensibly an array of char, or failing that that your implementation does, or that there's a compiler directive to request it.

You'll need to be careful to construct and destruct any member data placed into the buffer (I suggest you use placement new and explicit destruction for the entire struct - giving it constructors if convenient - rather than assigning to individual members), and should maintain static asserts to ensure the buffer is large enough.

The default copy constructor/operator should work just file because it'll just copy over the array contents. There's no need for a destructor because there's no managed resources in the class.

True for you right now perhaps, but do you really think another programmer is sure to check what you've done carefully when they just want to add a std::string, shared pointer or something?

Also are there any methods to make it less... well... ugly and more easily maintained?

Nothing worthwhile comes to mind.