Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am writing an audio engine that must be multi-platform (win/linux). Basically, a CSound represents a sound file, and CSoundInstance represents one playing of a CSound. Obviously, playing sound is very different on Windows than it is on linux.

A CSoundInstance needs access to some data from its corresponding CSound. The data is heavily implementation-dependent and cannot really be abstracted.

Here are the API headers for the two classes. I omitted operator= overloads / unique_ptr use etc.


class CSound
    CSoundImpl *impl;


class CSoundInstance
    CSoundInstance(CSound *); // going to need access to source voice buffer
    CSoundInstanceImpl *impl;

I guess it is clear where this is going. You would first instanciate a CSound, then acquire a CSoundInstance and Play it.

I Intend to have a specific XXImpl class for each system and platform. Under Windows, I use XAudio2, which needs several members in CSoundImpl (buffer + waveform descriptor), as well as CSoundInstanceImpl (voice + fx + sends + callbacks).

Here are the problems that I had using the pimpl idiom:

To play a sound, a CSoundInstanceImpl ultimately needs a reference to a CSoundImpl, on Windows and linux, but I cannot write this constructor:

CSoundInstance::CSoundInstance(CSound *sound)
    : impl(sound->impl)

Not only is sound->impl private, but it would also mess up unique_ptr use.

Another thing I tried is not using the constructor CSoundInstance(CSound *) at all, and adding a factory method with the signature CSoundInstance CSound::CreateInstance(), like this:

CSoundInstance CSound::CreateInstance()
    return this->impl->CreateInstance();

CSoundInstance CSoundImpl::CreateInstance()
    // here I have access to the CSoundImpl instance, that I need: this
    CSoundInstance instance(this);

But that would imply that I have this constructor:

CSoundInstance::CSoundInstance(CSoundImpl *soundImpl)
    : impl(new CSoundInstanceImpl(soundImpl))

Using a pointer to an implementation in the constructor of a non-implementation API class doesn't look right to me.

Maybe pimpl is not the right thing for this? Is there a way how I could tackle this problem better?

share|improve this question

closed as off-topic by Jamal Jul 18 '15 at 22:37

If this question can be reworded to fit the rules in the help center, please edit the question.

to me this sounds like a job for inheritance. You're asking for 2 different ways to do the same thing. So you would still use CSound, and CSoundImpl but those would be your parent classes. Make children classes of that, one for linux, and one for Windows. Then just have some simple logic to choose the correct child class, but use the parent class' methods to play/record your sounds. – Robert Snyder Feb 3 '13 at 21:41
@RobertSnyder That sounds reasonable. I've heard from a colleague about using pimpl as an alternative to interfaces in C++ (unfortunately, my area of expertise is C#, and low level C), so I just tried it and didn't really question that. You should probably make an answer from your comment though. – dialer Feb 3 '13 at 22:06
i can do that. Funny that you say that C# is your strength as that is my strength as well. C++ is new to me. I've not heard of pimpl so I don't want to discard it without knowing more about it, or it's principle. – Robert Snyder Feb 3 '13 at 22:13
I'm voting to close this question as off-topic because it is mostly for a design review. – Jamal Jul 18 '15 at 22:37
up vote 1 down vote accepted

As you said, if you need something in a pimpl class from another class, then something is wrong. If you can't abstract away the differences, then you can't hide the differences and shouldn't use this idiom.

Don't you think it would be simpler to put Play() in CSound? Each version (Linux/Windows) would have its own pimpl and there would be no need to try to find an abstraction or to try to access private data. Otherwise it gets a bit messy, since your Linux/Windows implementations of CSoundInstances will need to accept only the correct implementation: I don't know how easy it is to do this at compile time.

share|improve this answer
Your first sentence sums it up basically. I tried to use pimpl inappropriately. - But no, you really can't put Play() in CSound. CSound contains all static audio data ("the samples", up to several MB), and CSoundInstance contains playback specific parameters like effects, volume, panning, voice sends etc. Both are required for playback, and the same sound file can obviously be played multiple times simultaneously, with different parameters. – dialer Feb 5 '13 at 18:32
Couldn't the play() method return the playing sound? – Amy Blankenship Feb 6 '13 at 0:55

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