I am working with a function called CreateSourceVoice that takes a pointer to a pointer as its first argument. The definition is:

HRESULT CreateSourceVoice(IXAudio2SourceVoice **ppSourceVoice, ...);

The following code does not work. I get an error at the spot designated by the carets stating that the expression must be an l-value or a function designator.

class SourceVoice
{
public:
	IXAudio2SourceVoice* GetSourceVoice() { return sourceVoice; }

private:
	IXAudio2SourceVoice* sourceVoice;
};

SourceVoice musicVoice;

void SoundSystem::Play(Sound& sound)
{
	XAudio2->CreateSourceVoice(&(musicVoice.GetSourceVoice()), ...);

	                         ^^^^^
}

I can easily fix this by creating another function called GetSourceVoiceAddress like below and it works fine.

class SourceVoice
{
public:
	IXAudio2SourceVoice* GetSourceVoice() { return sourceVoice; }
	IXAudio2SourceVoice** GetSourceVoiceAddress() { return &sourceVoice; }

private:
	IXAudio2SourceVoice* sourceVoice;
};

SourceVoice musicVoice;

void SoundSystem::Play(Sound& sound)
{
	XAudio2->CreateSourceVoice(musicVoice.GetSourceVoiceAddress(), ...);
}

Or, if I store it in another pointer variable, it works fine.

void SoundSystem::Play(Sound& sound)
{
	IXAudio2SourceVoice* tempVoice = musicVoice.GetSourceVoice();
	XAudio2->CreateSourceVoice(&tempVoice, ...);
}

My question is what is fundamentally wrong with the top example, and why do the bottom two examples work when it seems like they are all doing the same thing. I am vaguely familiar with r-values and l-values, but obviously not enough.

docyoung83 said:
My question is what is fundamentally wrong with the top example

I assume your function CreateSourceVoice() allocates memory for some audio sample, and sets the pointer to this memory.

This means the pointer needs to be changed, so the function needs the address of the pointer.

But using this code:

XAudio2->CreateSourceVoice(&(musicVoice.GetSourceVoice()), ...);

The function does not get the address of the pointer. It only gets teh memory the pointer currently points to.
You try to fix this by adding one more &, but it won't work as expected. Your object SourceVoice will not know about the newly allocated memory, so you will be unable to free it.

So basically the compiler has helped to to avoid this mistake, sort of.

docyoung83 said:
why do the bottom two examples work when it seems like they are all doing the same thing.

Here you create a temporary variable pointing to the current memory.
But then your function CreateSourceVoice() will allocate new memory, and your temporary variable will point to it.
So you could free it as log as your temporary variable exists.
But your object SourceVoice will again not know about the newly allocated memory, only the temporary variable will.

So basically, the compiler tried to help you but you refused to take the helping hand, and managed to do it wrong again regardless. : )

IXAudio2SourceVoice** GetSourceVoiceAddress() { return &sourceVoice; }

This is the correct way to do it.
Because now your SourceVoice will know about the newly allocated memory. It's pointer will be changed, not some temporary copy of said pointer.
So now you can later free the memory using your object, and i assume only this way is what you really want and have in mind.

docyoung83 said:
I am vaguely familiar with r-values and l-values, but obviously not enough.

Same here, so i guess there should be a shorter explanation. But i hope it's clear anyway.

… there is however a way to do it like you originally wanted.
I had to compile and run it to verify due to confusing syntax, but i think it works:

class SourceVoice
{
public:
	int* GetSourceVoice() { return sourceVoice; }
	int** GetSourceVoiceAddress() { return &sourceVoice; }

	int*& GetSourceVoice_() { return sourceVoice; } // meant to replace both of above two functions

	SourceVoice ()
	{
		sourceVoice = new int [100];

		int** pptr = GetSourceVoiceAddress();
		int** pptr2 = &GetSourceVoice_();

		GetSourceVoice()[0] = 1;
		GetSourceVoice_()[0] = 2;
		assert (pptr == pptr2);
	}

private:
	int* sourceVoice;
};

So buy returning a reference to a pointer, only the function with the underscore is needed.

But not sure if this is good practice and if there is a better way to be more verbose.

Awesome! Thank you very much. It makes sense now.

But my proposal is fishy. Someone could change the returned pointer, unaware that this also changes to pointer in the object, so it's error prone.

But currently i fail to come up with the better way. I'm sure some someone else responds… : )

JoeJ said:
So buy returning a reference to a pointer, only the function with the underscore is needed. But not sure if this is good practice and if there is a better way to be more verbose.

If one insists on doing it that way, I would definately return a *& instead of a ** like you did there. Having a reference when the value cannot be null is preferably in my opinion.

Though, in terms of best practice, eposing and modifying a variable with a getter that returns a pointer is kinda of weird, at least in my book. If you (=OP) really don't care about encapsulation (that is, you have methods for fully getting and effectively setting the variable), you can just make it public. Preferably, I would design the class to where eigther the constructor takes the IXAudio2SourceVoice, or even better create it inside the constuctor or an Init-method of sorts. Than you don't need to expose any direct setter. Then instead of calling the raw XAuido-functions with their weird (or at least verbose) syntax you have a a set of well-defined functions that can be much more simple for what you need the to do.

Ah yes, agree. I thought i remember something better, but found i confused this with something else.

So, while we have a working solution technically, it's bad practice. Sometimes such things can't be avoided, but if fishy smell comes up, i would try to redesign if possible.
In this case, using CreateSourceVoice() should be done by the object SourceVoice, eventually. Then ownership is clear and there is no need to expose the address of the pointer.

Though, this causes dependency on a specific API in an object which maybe is meant to be more general. Then we can either create an abstraction of the API (some work), or do as proposed, concluding that's the lesser evil then.

@JoeJ

JoeJ said:
In this case, using CreateSourceVoice() should be done by the object SourceVoice, eventually. Then ownership is clear and there is no need to expose the address of the pointer.

That's always something I have a hard time deciding. Sometimes I wind up trying to mash everything together into one giant superclass that does everything, but I've been trying to be more modular and encapsulate things into their own areas. I sort of have 3 main components of the “sound system".

1. SoundLibrary: Just a big collection of individual Sound structs, each with a SOUND_ID and the raw audio data. No functionality, just storage.
2. SourceVoice: The pipe through which the Sound from the SoundLibrary gets sent to the SoundSystem.
3. SoundSystem: The speaker that actually plays the sound.

Sound			SourceVoice
Sound			SourceVoice
Sound  ==> Sound Library  ==>	SourceVoice  ==>  SoundSystem
Sound			SourceVoice
Sound							
Sound
Sound
...
...

With XAudio2, each sound has to have its own SourceVoice for it to be played. The same SourceVoice can be reused to play other sounds, but a SourceVoice can only play one sound at a time. So you can't play more sounds at a single time than you have SourceVoices to support them. I initially had EACH SOUND with its own SourceVoice. So if you had 1000 sound assets, you'd have 1000 SourceVoices.

class Sound
{
	int sound_id;
	char* raw_data;
	SourceVoice voice;
};

But that has problems in that you can't start the same sound again until it has finished. So if you have a gunshot sound that needed to be played frequently enough that the sound would overlap, you couldn't do it.

So as it is now, I assume I will probably never have more than 100 sounds playing at once, so I basically just have 100 SourceVoice objects in a vector, and when I sound is sent for playing, the SoundSystem looks through the SourceVoice vector until it finds one not in use. And each time, it has to call that function CreateSourceVoice to “attach” the Sound to the SourceVoice. So I guess I just made the decision to have the SourceVoice ignorant of whatever SoundSystem needs to do. The most important object of SoundSystem is the Microsoft::WRL::ComPtr<IXAudio2> object that calls the CreateSourceVoice, and I guess I just wanted to keep it separate from the voices.

At least I think lol. I've only just started this so that could all be totally incorrect. XAudio2 seems to have a very limited amount of tutorials. But always interested to see how other people would go about designing things.

docyoung83 said:
That's always something I have a hard time deciding. Sometimes I wind up trying to mash everything together into one giant superclass that does everything, but I've been trying to be more modular and encapsulate things into their own areas.

Well, i'm not really the guy to give advice on that, because i never do any planning on software design at all. I think i have just learned it well enough from my own mistakes after decades. But i also still think it's not that important to get right from the start.

Usually i start with some superclass too, but i expect it grows and i have to subdivide it in the future. So i try to create multiple classes / data structures from the start, but keep them all in the same file for a while. It feels messy and amateurish. Compile times suffer, and Intellisense can't keep up with my too much code. Then i spread it out over multiple files, i feel better, and mostly it turns out things are pretty fine. Eventually i refactor some classes to become more general so i can use them in other places too.
To keep things modular, i only try to minimize dependencies. But i do not consciously aim for encapsulation or abstraction. I rather hope such good things just emerge automatically where possible.

docyoung83 said:
I guess I just wanted to keep it separate from the voices.

Sounds reasonable. Likely you have some reason for this feeling, which then is probably more important for the case than some abstract design principles. You can change it later easily if it feels bad at some point.

I have only little experience with audio systems. I have used OpenAL in past which probably is pretty similar. The main problem with many sounds at once for me was to prevent clipping, which i had solved with some naive heuristic approach of reducing volume gradually as count of sounds increases. There should be a better way of mixing ahead of time and analyzing the summed signal, but OpenAL seemingly had no functionality to help with such approach.