Sign in to follow this  
Tispe

How do I properly return a private class member vector using move semantics?

Recommended Posts

Hi

 

I have a class named "Window" that has a private vector of messages called m_windowMessages.

This class has a public method named "moveMessages" and I want it to return the private vector of messages using move semantics without messing up the private vector so that I can continue to push new messages on it after "moveMessages" returns.

 

But I don't know what solution to use, I find some of them buggy, I think m_windowMessages get mangled and it has to renew its internals. Could the std::tuple ble the problem, should i just use a struct?

class Window
{
public:
	Window();
	~Window();
	bool initialize(HINSTANCE hInstance);

	std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> moveMessages();

	bool postMessage(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam);

private:
	bool filterMessage(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam);

	HWND m_hWnd = nullptr;
	HINSTANCE m_hInstance = nullptr;
	WNDCLASSEX m_wc;

	std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> m_windowMessages;
};

Window::Window()
{
	ZeroMemory(&m_wc, sizeof(WNDCLASSEX));
	
	m_wc.lpfnWndProc = (WNDPROC)[](HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)->LRESULT		// non-capture lambda
	{
		Window* p_Window = nullptr;
		if (message == WM_NCCREATE)
		{
			p_Window = reinterpret_cast<Window*>(((LPCREATESTRUCT)lParam)->lpCreateParams);
			if (p_Window == nullptr)
				throw;

			SetLastError(0);
			if (SetWindowLongPtr(hWnd, GWLP_USERDATA, reinterpret_cast<LONG_PTR>(p_Window)) == 0 && GetLastError() != 0)
				throw;
		}

		if ((p_Window = reinterpret_cast<Window*>(GetWindowLongPtr(hWnd, GWL_USERDATA))) == nullptr)
			return DefWindowProc(hWnd, message, wParam, lParam);

		if (p_Window->postMessage(hWnd, message, wParam, lParam))
			return 0;

		return DefWindowProc(hWnd, message, wParam, lParam);
	};
}
std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> Window::moveMessages()
{
	MSG msg;

	while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
	{
		TranslateMessage(&msg);
		DispatchMessage(&msg);
	}

	// What to do??

	return std::move(m_windowMessages);		// Mangles m_windowMessages causing application to be laggy

	std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> windowMessages;
	std::swap(windowMessages, m_windowMessages);
	return windowMessages;				// Kinda laggy

	return std::move(windowMessages);		// App also stutters here
}

bool Window::postMessage(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
{
	if (filterMessage(hWnd, message, wParam, lParam))
	{
		m_windowMessages.push_back(std::make_tuple(hWnd, message, wParam, lParam));
		return true;
	}

	return false;
}

bool Window::filterMessage(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
{
	if (hWnd != m_hWnd) return false;

	// return true for messages I want keep on m_windowMessages for the caller of moveMessages() to handle later on
	switch (message)
	{
		case WM_MOUSEMOVE: return true;
		case WM_LBUTTONDOWN: return true;
		case WM_LBUTTONUP: return true;
		case WM_CHAR: return true;
		case WM_SYSKEYUP: return true;
		case WM_SYSKEYDOWN: return true;
		case WM_DESTROY: return true;
		case WM_SYSCOMMAND:
			switch (wParam)
			{
				case SC_KEYMENU: return true;		// Disable system menu (F10 Key)  
				case SC_MONITORPOWER: return true;	// Disable power saving monitor shutdown
			}
			break;
	}
	return false;
}
Window MyWindow;
MyWindow.initialize(hInstance);

while(true)
{
	auto newMessages = MyWindow.moveMessages();

	/.../
	// processing newMessages
	/.../
}
Edited by Tispe

Share this post


Link to post
Share on other sites
I question how you are seeing lines as 'laggy' however...

When you tell it to 'move' the vector you are saying "I no longer want the vector you are moving from to be valid" which means it gives up everything, including the memory buffer it was pointing at.

When you next 'push back' it has to reserve new memory to fill all over again.

Assuming you've profiled this properly NOT in debug and found this to be a problem then I would suggest either;
a) reserving at the start of the processing loop a sane amount of memory
b) not moving the vector out, instead return const_iterators and processing the messages using that to iterate before clearing the buffer at the start of the next frame.

Share this post


Link to post
Share on other sites

Hi, thx for reply, but didn't help :(

 

Reserving does not fix the issue.

I want the messages out of Window without deep copy. I do not want interators pointing into private members, what if the Window is destroyed while I use the iterators?

 

I have changed some of the code for more testing, such that m_windowMessages is now a pointer to a vector, then I assign the pointer the address of a local vector, run PeekMessage loop which will post messages using the pointer, then try to return by rvalue. But no luck! :(

 

Funny enough, if I do a deep copy there is no lag hah, but then m_windowMessages just builds up to have thousands of messages (mostly WM_MOUSEMOVE) just in a few seconds.

 

I think the problem is the "return std::move(.....)" is not working properly, something I got wrong. Is it even possible to return rvalue? How do the caller properly fetch rvalue to invoke the move constructor: "auto windowMessages = m_Window->moveMessages();"?

Edited by Tispe

Share this post


Link to post
Share on other sites

This one is correct:

std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> windowMessages;
std::swap(windowMessages, m_windowMessages);
return windowMessages;				// Kinda laggy

The reason why it is better than returning an rvalue, is because while you are avoiding a COPY by doing a MOVE, you are actually (possibly) getting in the way of the compiler from doing neither a move nor a copy - RVO (return-value-optimization) permits compilers to make "windowMessages" actually not exist, and assign directly to the variable that is receiving the return value.

i.e. 'meow' in:    meow = blah.moveMessages();

You say it's "kinda laggy", but it shouldn't be laggy - it should be very fast (internally it should swap a few pointers and integers). So you need to profile and debug to see why it's laggy. Likely your lag is unrelated to that particular piece of code (though I'd move "auto newMessages" to outside the loop, so you can help hint to the compiler to re-use it over - the compiler should be able to figure it out, but force of habit on my part, and a good sanity check).

 

Also, I'd rename the function "takeMessages()", since I believe "take" is the more commonly-used word for this kind of scenario (take = get and claim ownership / remove from current owner), but maybe that's just in my neck of the woods. Occupied solely by me.  :P

 

Your swap (two moves) might be unnecessary there. I'd do it like this:

std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> windowMessages = std::move(m_windowMessages);
m_windowMessages.clear();

return windowMessages;

That should be kosher, but if std::vector has a stupid implementation, it might not be - in which case you can go back to the one above.

 

Oh, and in answer to your other question, yes you can return r-values, but unless you have a reason to, you should prefer returning l-values, because the compiler's RVO is superior and returning by r-value might trigger the compiler to disable its RVO to explicitly follow your request.

 

Two examples of standard library functions that return by r-value are the r-value casting functions: std::move() and std::forward().

Share this post


Link to post
Share on other sites

Hi, thanks for your input.

 

I might change the name to "takeMessages()" :)  , I was thinking of maybe using "getMessages()" but that could be interpreted/confused as a deep copy, leaving copies behind.

If I do manual variable hoisting if I don't trust the compiler to do it, then I still would have to clear it each time inside anway, so trust the compiler to do it and I save 1 line of code  :lol:

 

So, if I stay away from std::move and && return type declarations, the compiler might return vector by rvalue anway right?

Is there a chance the compiler will return by value(deep copy) in that case?

What is the guarantee it will never do deep copy in this case?

 

Also, is there a size where the return vector is large enough for rvalue to outperform RVO? In this case, will this be resolved run-time and selected as appropiate, or is it compiled either as RVO or rvalue move?

Share this post


Link to post
Share on other sites

I might change the name to "takeMessages()"   , I was thinking of maybe using "getMessages()" but that could be interpreted/confused as a deep copy, leaving copies behind.


Yes, getX() would be misleading; forming and adopting common nomenclature for functions is helpful in coding. get() implies a deep copy (usually just for small types) or the return of a handle/reference, whereas take() implies getting and removing from the original owner, often with whatever optimizations can benefit.
 

If I do manual variable hoisting if I don't trust the compiler to do it, then I still would have to clear it each time inside anway, so trust the compiler to do it and I save 1 line of code 

Since you are assigning to it, you don't have to clear it.
 

So, if I stay away from std::move and && return type declarations, the compiler might return vector by rvalue anway right?

No, it'll very likely do something better than rvalue.
 

Is there a chance the compiler will return by value(deep copy) in that case?


Sure, if you are using a terrible compiler, or a compiler version more than five years old, or if your good modern compiler decides not to perform RVO in this specific function for some reason - which you then handle on a case-by-case basis.
 

What is the guarantee it will never do deep copy in this case?

No guarantee. :P The same no guarantee that C++ intentionally has available that permits C++ to be faster than every other language out there.  :wink: 
The standard intentionally does not dictate how compilers do things, the standard merely says what the result should be, and intentionally gives compilers the elbow-room to do optimizations.

An example would be v-tables or empty-base optimization. Or small-string optimization. The standard doesn't dictate any of them, afaik.

There are optimizations I generally trust because they are so widespread, and optimizations I generally don't until I test to make sure. RVO is one I assume is present until (on a case-by-case basis) it is proven otherwise.
 

Also, is there a size where the return vector is large enough for rvalue to outperform RVO?

Nope, never. Provided a basic level of sanity in the implementation of your compiler's RVO.

In fact, that's an odd question, because even between two rvalue moves, the size of a vector is mostly irrelevant. Provided a basic level of sanity in the implementation of std::vector, it is equally fast to move a million-element vector as it is to move a single element vector - unlike with copying.
But RVO will be faster than the move. If the compiler does RVO (which is common if you have a single return in your function), it is the fastest possible solution. The fastest code is no code, and RVO is a way for the compiler to remove as much code as possible and still get the same result as returning an rvalue or lvalue.

NOTE: "provided a basic level of sanity in the implementation" is an assumption I always have to make or reject for anything in C++; with RVO, we're talking about the compiler, with std::vector's move-constructor/move-assignment, we're talking about whichever standard library implementation you are using (hint: three or four widely-used different implementations of the standard library exist).
 

In this case, will this be resolved run-time and selected as appropiate, or is it compiled either as RVO or rvalue move?

At compile-time, because compile-time is faster than run-time. Further, no decision at run-time is needed - RVO is faster. Always.  :P

 

The question is only, "is the compiler choosing to do it, or not?".

 

Well, no, the question really is, "is my function running fast enough, or not?", and if the answer is, "No, it's too slow", then the next question is, "Why is it slow?".

 

Returning a vector by RVO, or by move-semantics, should be very fast! Especially since you're only doing it once per frame. Heck, even if you were doing a hundred thousand times a frame, it should still be fast. Because you are saying it's lagging, something in your code somewhere is wrong, and I can't see it in this:

std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> windowMessages;
std::swap(windowMessages, m_windowMessages);
return windowMessages;

So I'd look elsewhere first.

 

I'd check in here first:

while (PeekMessage(&msg, NULL, 0, 0, PM_REMOVE))
{
	TranslateMessage(&msg);
	DispatchMessage(&msg);
}

(Also, since you are doing event processing inside a function that, by its name, implies it's just getting something and quickly returning, neither move() nor take() works as a good name in that regard. More like "processMessages()". I hadn't realized that the first time around)

Share this post


Link to post
Share on other sites

If I do manual variable hoisting if I don't trust the compiler to do it ...
Compilers are engineered to optimize for the common case that everybody writes. By trying to write smart code, you're writing unusual code, thereby disabling a lot of the heuristics in the compiler for recognizing cases that can be optimized by doing something completely different, you'd never ever invent by yourself. (Or if you could, you may not be able to express it in C++.)

 

In about 99% of the cases, writing simple straight-forward code is the way to go, giving the compiler optimal room for performing its smart tricks.

Share this post


Link to post
Share on other sites

RVO is faster. Always. 
 

 

I think I remember watching a video where Scott Meyers or the MS compiler guy said something about RVO, where if the return size is larger than 4k~ then RVO could be worse then rvalue since the caching/paging mechanisms of the stack would not be large enough for RVO to function without the CPU fetching more pages to the cache. Afaik the retern value has to be at the top of the stack after the function returns, and if the pre-fetcher don't know how much memory the RVO will use on the stack then it would have to reallocate for every page it has to grab. Do you think that applies to returning a std::vector since internals is pointing to heap?

Share this post


Link to post
Share on other sites

I made a mistake in my first post, where I said: "I'd move "auto newMessages" to outside the loop, so you can help hint to the compiler to re-use it"
I had forgotten that for RVO to take effect, you have to construct the variable from the result of the function (not simply assign), so the variable can't be brought outside the loop (oops) if you also want RVO - in that case, I'd still bring it outside the loop (saving the repeated constructions/destructions).
 

 

RVO is faster. Always.

 
 
I think I remember watching a video where Scott Meyers or the MS compiler guy said something about RVO, where if the return size is larger than 4k~ then RVO could be worse then rvalue since the caching/paging mechanisms of the stack would not be large enough for RVO to function without the CPU fetching more pages to the cache. Afaik the retern value has to be at the top of the stack after the function returns, and if the pre-fetcher don't know how much memory the RVO will use on the stack then it would have to reallocate for every page it has to grab. Do you think that applies to returning a std::vector since internals is pointing to heap?

 

 
Seeing that the compiler can do whatever it wants, as long as it gets the same result, we can't say for sure what it's doing in your case without seeing the assembly. (and in my case, I haven't yet taken time to learn assembly.  :ph34r:)
 
That said, we can theoretically walk through both cases of RVO and move-semantics. Let's say your vector contains 1 MB of data:

struct MyElement
{
   uint32_t x,y,z,w; //16 bytes.
};

template<typename Type = MyElement>
class MyVector //Our pretend implementation of std::vector
{
    size_t elementCount = {67 million and change};
    Type *data = {1 MB of data};

public:
    MyVector(MyVector &&other) //Move constructor.
    {
        std::swap(elementCount, other.elementCount);
        std::swap(data, other.data); //Simply swaps the *pointer*, does *not* move around any data! Doesn't even deference the pointer (so that particular cache line isn't even called up).
    }

}; //Likely 16 bytes on a 64 bit OS (a 64 bit integer and a 64 bit pointer).

(Some implementations of std::vector use two pointers (begin and end) rather than 'begin' and 'count', because this is more optimized CPU-wise, but that doesn't affect this discussion, and the class size would end up being pretty much the same (two 64bit pointers vs a 64 bit pointer and a 64 bit integer)).
 
So now let's examine two theoretical straight-forward implementations of returning r-values vs RVO.
 
Returning r-value:

//------------------------
//Example situation:
//------------------------

MyVector&& MyFunc()
{
    MyVector localVector;
    std::swap(localVector, this->memberVector);
    
    return std::move(localVector);
}

MyVector finalVector = MyFunc();

//------------------------
//Example implementation:
//------------------------

Allocate finalVector //16 bytes
Call MyVector() default-constructor on finalVector

Allocate MyFunc_localVector //16 bytes
Call MyVector() default-constructor on newly allocated MyFunc_localVector

Swap MyFunc_localVector and MyFunc_memberVector
.....(three moves* and an MyVector temporary allocation) *[by 'move' I mean a call to move-assignment or move-constructor)
..........(each MyVector move is two swaps)
................(each swap is three moves and a temporary allocation)

Call Function std::move() on MyFunc_localVector
.....(std::move() is just a cast from l-value reference to r-value reference)
.....[remember: the function takes a universal reference, not a r-value reference - that can look confusing]
.....[this std::move() will very likely get optimized out entirely, unless I'm confused]

Return the result and assign to finalVector
.....Move [unnamed MyVector r-value] into finalVector
..........(each MyVector move is two swaps)
................(each swap is three moves and a temporary allocation)

Destruct [unnamed MyVector r-value]

//Later:
Destruct finalVector

If RVO takes affect, then it (possibly) looks like this: 

Allocate finalVector //16 bytes
//Skipped: Call MyVector() default-constructor on finalVector

//Skipped: Allocate MyFunc_localVector //16 bytes
Call MyVector() default-constructor on [[pre-existing]] [[finalVector]]

Swap [[finalVector]] and MyFunc_memberVector
.....(three moves* and an MyVector temporary allocation) *[by 'move' I mean a call to move-assignment or move-constructor)
..........(each MyVector move is two swaps)
................(each swap is three moves and a temporary allocation)

//Skipped: (in the other case, it'd be likely optimized out anyway)
//   Call Function std::move()
//   .....(std::move() is just a cast from l-value reference to r-value reference)
//   .....[remember: the function takes a universal reference, not a r-value reference - that can look confusing]
//   .....[this std::move() will very likely get optimized out entirely, unless I'm confused]

//Skipped:
//   Return the result and assign to finalVector
//   .....Move [unnamed MyVector r-value] into finalVector
//   ..........(each MyVector move is two swaps)
//   ................(each swap is three moves and a temporary allocation)

//Skipped: Destruct [unnamed MyVector r-value]

The only problem in this case is that RVO might not get triggered.
 
But here's good news: If RVO isn't triggered, code like this:

std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> Window::moveMessages()
{
	//...blah...

	std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> windowMessages;
	std::swap(windowMessages, m_windowMessages);
	return windowMessages;
}

...that's used like this:

auto newMessages = MyWindow.moveMessages();

...has newMessages' move-constructor getting called anyway, because the result of that function is still an rvalue!
 

MyType x = (y + z); //This is returning an rvalue and giving it to 'x'.

As you know, '(y + z)' is really a function call: 'operator+(y, z)'. Yet the result is an rvalue, because once the function exits, there's no existing variables connected to the value.
In the same way, MyWindow.moveMessages()'s result is an rvalue. It's an unnamed temporary variable that the compiler knows isn't being used by anything else.

Returning by value will either RVO (when possible), or rvalue (if move-constructor/move-assignment operators are available for that type), or deep copy if your compiler doesn't support move-semantics (and if your compiler doesn't support move semantics, calling std::move() ain't gonna make a difference! :wink:).
 
But anyways, regardless of whether the compiler uses RVO is used or move-semantics, the 1MB data in the vector isn't touched.

So this shouldn't be the location of your slowdown, unless I'm missing something (which is very possible! :P).

Have you really confirmed through profiling that your slowdown is in how you return from that function?

Share this post


Link to post
Share on other sites
For the record; returning a local variable via move semantics is, per Effective Modern C++ (for C++11 & 14) by Scott Meyers, a bad idea and will more than likely disable RVO/NRVO and force the compiler in to a move instead of doing clever in place initialisation of the returned value.

Share this post


Link to post
Share on other sites

In this particular case, the most efficient solution is probably the C++98 solution:

void takeMessages(std::vector<std::tuple<HWND, UINT, WPARAM, LPARAM>> & windowMessages)
{
    std::swap(windowMessages, m_windowMessages);
    m_windowMessages.clear();
}

As long as the user doesn't throw away his vector every loop, memory will actually be reused every time instead of being reallocated. (which will happen no matter if you use RVO-move or regular move into another vector)

Share this post


Link to post
Share on other sites

I have a similar follow up question about returning a std::tuple<ve0, vec1, vec2, vec3, ....>.

 

Suppose a game class have several member vectors:

class Game
{
public:
	Game();
	~Game();
	std::tuple<
		std::vector<std::string>,			// vec0
		std::vector<std::string>,			// vec1
		std::vector<std::string>,			// vec2
		std::vector<std::string>			// vec3
	> takeOutput();
private:
		
	//Wrap members in m_output?
	//std::tuple<
	std::vector<std::string> m_vec0;
	std::vector<std::string> m_vec1;
	std::vector<std::string> m_vec2;
	std::vector<std::string> m_vec3;
	//> m_output
}

The game class has a method to take out these vectors called std::tuple<....> takeOutput():

std::tuple<
		std::vector<std::string>,			// vec0
		std::vector<std::string>,			// vec1
		std::vector<std::string>,			// vec2
		std::vector<std::string>			// vec3
	> Game::takeOutput()
{
	return m_output;	// returns a copy? I want to move it out

	//? return std::move(m_output);	

	//std::swap(localTuple, m_output) return localTuple;	//swap method with a local, disables RVO?

	

	//? return std::make_tuple(m_vec0, m_vec1, m_vec2, m_vec3);

	//? return std::make_tuple(std::move(m_vec0), std::move(m_vec1), std::move(m_vec2), std::move(m_vec3));
}

I believe std::tuple has move semantics built in. But how would I go about wrapping the vectors in the tuple before returning from takeOutput()?

Should I have a member std::tuple<...> m_output and return it, possibly using std::swap or std::move?

 

Or should I return r-value using std::make_tuple<...>(vec0, vec1, vec2, vec3....)? What about std::move to move vectors into tuple?

Edited by Tispe

Share this post


Link to post
Share on other sites

I would say the similar problem requires you to answer similar questions. Do you need the vectors, and/or the tuples, to be in a usable state after you take the data? If so, then the std::swap approach seems to be the correct answer - anything else will just replicate the std::swap process manually, or do something even less efficient. You can create the tuple, swap the vectors in, then return the tuple. You don't need to 'move' it out, because (a) - that's what RVO is there for, and (b) since the tuple will be destroyed, more recent compilers can see that it's going out of scope and can generate the moves for you.

 

Personally this is starting to look like a pretty bad code smell though. If you're heavily reliant on pushing things into a structure, only to pull them out again en-masse, it sounds like you either want some sort of queuing system or a double-buffer system.

Share this post


Link to post
Share on other sites

I watch videos from CPPCON15/16 and they keep saying, "return it by value" etc. Also the using refs or pointers to get output through arguments kinda seem to conflict with the idea of "returning". It seems to me that the "propper" use of the language is to use arguments as inputs, and the return as output... I don't want to create "bad code" at all, I just want good code, and I need to know how the language intends for me to return loads of data. It seems the move semantics are for this exact purpose. And to return anything more then one value I need to wrap it up in tuple or struct. And bonus STL containers has the move semantics built in so I don't have to worry about writing bad && operations.

 

There are so many parts to move semantics along the way that I feel I could get wrong, and if I mess up just one place I will be in deep copy hell and have a hard time figuring out where to apply the fix.

 

I could take a referenced tuple as a argument and std::swap with that, but... it just seems wrong.

Share this post


Link to post
Share on other sites

As someone who is not yet up-to-date with modern C++ and move semantics, I understand your pain. But I think you've made things appear more complex than they actually are, due to your desire to implement queues/buffers via this sort of mechanism. Move semantics are for transferring ownership of an object. But when you move a vector, you move the whole vector, not just the contents. That's why it's the wrong solution for your buffer/queue situation. You need to keep a vector around in preparation for the next load of data - and that's where the old std::swap trick comes in - it basically does exactly the same as a std::move except it keeps your vector valid. And you can return the vector by value because the compiler will work out how to do that efficiently. (C++ compiler writers have been aware of this issue and did things to fix it well over a decade ago.)

 

Also, don't sweat these details until you know there's a problem. How many windows messages are you expecting per frame? 1? 2? Perhaps 10 if someone is particularly vigorous with the mouse? The amount of data being copied here is likely to be so small that copying it all might even be the cheapest approach. Allocating and deallocating vectors might be more expensive.

Share this post


Link to post
Share on other sites
Move semantics are for transferring ownership of an object. But when you move a vector, you move the whole vector, not just the contents. That's why it's the wrong solution for your buffer/queue situation. You need to keep a vector around in preparation for the next load of data - and that's where the old std::swap trick comes in - it basically does exactly the same as a std::move except it keeps your vector valid. And you can return the vector by value because the compiler will work out how to do that efficiently.

 

Yes but is it the way the language intended to "transfer/move" data?

If returning a vector by value, what about this post above:

 

 

For the record; returning a local variable via move semantics is, per Effective Modern C++ (for C++11 & 14) by Scott Meyers, a bad idea and will more than likely disable RVO/NRVO and force the compiler in to a move instead of doing clever in place initialisation of the returned value.

Is that only because I was thinking about returning using std::move?

 

Would this be "proper" c++

std::tuple<
		std::vector<std::string>,			// vec0
		std::vector<std::string>,			// vec1
		std::vector<std::string>,			// vec2
		std::vector<std::string>			// vec3
	> Game::takeOutput()
{
	std::tuple<
		std::vector<std::string>,			// vec0
		std::vector<std::string>,			// vec1
		std::vector<std::string>,			// vec2
		std::vector<std::string>			// vec3
	> localTuple;

	std::swap(std::get<0>(localTuple), m_vec0);
	std::swap(std::get<1>(localTuple), m_vec1);
	std::swap(std::get<2>(localTuple), m_vec2);
	std::swap(std::get<3>(localTuple), m_vec3);

	return localTuple;
}
Edited by Tispe

Share this post


Link to post
Share on other sites

The C++ language doesn't have intent; it's a programming language, not a person. It provides tools for you to move data around in a method that suits you. Some of those methods are simple, some are fast, and some are in-between. The recent move/rvalue stuff is designed to give you an extra tool which strikes a better compromise between speed and complexity in some situations. (In the best case, you only get the extra speed, and in the worst case, you only get the extra complexity.)

 

std::move can get in the way of the compiler performing Named Value Optimisation, which, if you need to return something by value, is about as optimal as it gets, as it can avoid extra copies.

 

Regarding the question of whether your code is 'proper' C++; it looks to be legal and will perform your intended task.

Is it good C++? I'd say no; it looks ugly, and you've not convinced me that this is really something that you should be doing.

Is it optimal C++? Again I'd say no; even though you avoid copying strings, every call to takeOutput creates 4 new vectors, every subsequent object added goes in a completely new bit of memory (bad for the cache), and each vector (presumably) gets destroyed by whatever function called takeOutput once it is done with the contents.

 

Ultimately I would argue that it's not possible to give you the 'right' answer in the abstract here: it depends on how it will be used. That's why there are a bunch of different potantial approaches available. But it's probably no coincidence that existing libraries that wrap the OS message system (as in your original example), just yield up one message at a time - no copying of lists, no new data structures being allocated and deallocated, just writing the very simple message data in-place. (eg. https://wiki.libsdl.org/SDL_PollEvent, http://www.sfml-dev.org/documentation/2.0/classsf_1_1Window.php)

Share this post


Link to post
Share on other sites
Yes but is it the way the language intended to "transfer/move" data? If returning a vector by value, what about this post above:

The C++ language assumes you understand what you're doing or not doing. It doesn't make choices for you, instead it mostly gives you primitives (options that you can use at your discretion), and very little policy or guidelines.

 

Even if it wanted to, it can't give guidelines in the general case, as "good use" is very different at different processor architectures, or even different machines.

Eg, a 16bit singleboard embedded system, versus an x64 desktop system, versus a Cray supercomputer.

 

 

In other words, you decide what is "good" for your case. Optimally, you do that based on knowledge from the system that you'll use for the software. In case of 'move' semantics, there is likely a break-even point somewhere, as I would guess returning a boolean is better done by value, and returning a 1GB vector is better done by move/swap.

If you don't know or don't care, just use the simplest solution until you find a counter-example where it makes a significant difference. At that time, adjust your view based on the experience.

 

 

Edit: Ninja'd by Kylotan

Edited by Alberth

Share this post


Link to post
Share on other sites
In this case you might be able to "get away with" doing the following;

std::tuple<vec, vec, vec, vec> Game::takeOutput()
{
   return { std::move(vec1), std::move(vec2), std::move(vec3), std::move(vec4) };
}
Which indicates you don't care about the content of vec1...4 so move them in to the tuple, then return that by value which might allow RVO to kick in and directly move those vectors to the return value...

maybe.

(but, then again I agree with the above that this feels like a bad code smell and probably not a sane way to go about doing things...)

Share this post


Link to post
Share on other sites

In this case you might be able to "get away with" doing the following;
 

std::tuple<vec, vec, vec, vec> Game::takeOutput()
{
   return { std::move(vec1), std::move(vec2), std::move(vec3), std::move(vec4) };
}
Which indicates you don't care about the content of vec1...4 so move them in to the tuple, then return that by value which might allow RVO to kick in and directly move those vectors to the return value...

maybe.

(but, then again I agree with the above that this feels like a bad code smell and probably not a sane way to go about doing things...)

 

 

Very interesting, but I think the issue begins when reusing the member vectors after they have moved.

 

Is it optimal C++? Again I'd say no; even though you avoid copying strings, every call to takeOutput creates 4 new vectors, every subsequent object added goes in a completely new bit of memory (bad for the cache), and each vector (presumably) gets destroyed by whatever function called takeOutput once it is done with the contents.

I could avoid creating 4 new vectors every time the method is called, but I would have to pass in the old ones and do the swaps. And they would have to be cleared before being reused. Is that not a "code smell"? Assuming a game refresh rate of 120 hz, are 480 vector creations per second "that bad" on a modern intel cpu? Maybe the compiler reuses behind the scenes?

 

Some of the std::strings in this example are placeholders for later when they will get swapped out by data such as "network data", "asset requests", "displaySettings" etc. I would also expand the output by adding more vectors to the tuple as the game gets more mature. I'm thinking the Game class should only keep track of game data and return only what the graphics class, window class, network class etc would need from it.

 

I'm not sure but from what I have learned I think this ain't all that bad:

class Game
{
public:
	Game();
	~Game();
	void postInput(/*some vectors or a tuple maybe lol*/);
	void update();
	std::tuple<
		std::vector<std::string>,
		std::vector<std::string>,
		std::vector<std::string>,
		std::vector<std::string>
	> takeOutput();
private:
	std::tuple<
	std::vector<std::string>,		//m_vec0;
	std::vector<std::string>,		//m_vec1;
	std::vector<std::string>,		//m_vec2;
	std::vector<std::string>		//m_vec3;
	> m_output
}

void Game::update()
{
	// ...
	// produce output
	// ...
}

std::tuple<
		std::vector<std::string>,
		std::vector<std::string>,
		std::vector<std::string>,
		std::vector<std::string>
	> Game::takeOutput()
{
	std::tuple<
		std::vector<std::string>,
		std::vector<std::string>,
		std::vector<std::string>,
		std::vector<std::string>
	> localTuple;

	std::swap(localTuple, m_output);
	return localTuple;
}

Or do I just not "get it"?

Share this post


Link to post
Share on other sites
For what you are trying to do; no it isn't "bad".

But from what you've said what you are trying to do seems very very bad.

You seem to be trying to jam a bunch of unrelated information in to a structure and then return it to 'somewhere' via a single point... when that data probably has no reason to be there, never mind jammed together with unrelated stuff and passed about.

The key question here is what are you trying to do REALLY?
What problem are you trying to solve?

Because literally none of the data you talked about in the above post needs to be subject to this design torture...

Share this post


Link to post
Share on other sites
The key question here is what are you trying to do REALLY? What problem are you trying to solve?

 

 

I am trying to make a simple game beginning with a simple structure that I will expand on:

init
mainloop
exit

mainloop:
	input
	update
	output
	repeat

input:
	getInputWindows
	getInputNetwork

update:
	game->postInput
	game->update

output:
	output = game->takeOutput
	network->send(output.packets)
	graphics->render(output.graphics)
	audio->play(output.sound)

I'm not looking for something special, just a sensible design. Why do I get a sense that there is no sensible design, it seems that no matter how I code, there will always be "issues" with the code. I don't think I can avoid basic things such as passing data to another part of the application? And no, I am not going to have the game do draw calls directly. Why? becuase all the reasons that have put me in refactory hell in the past.

Edited by Tispe

Share this post


Link to post
Share on other sites
The basic structure you just posted is fine, but returning tuples of vectors is not idiomatic C++. What is the specific problem you're trying to solve here? Are you more used to functional programming languages? What you're doing would be idiomatic in a functional programming language - in particular, this looks a lot like the Erlang code I've seen - but not a lot of C++ code is written like this.

Typical C++ code would have those vectors accessed through "getter" functions that return references to the vectors in the class, and then the vectors are used directly by the caller. Eg.
 
class Game
{
public:
	Game();
	~Game();
	void postInput(/*some vectors or a tuple maybe lol*/);
	void update();
	
	std::vector<std::string>& get_vec0();
	std::vector<std::string>& get_vec1();
	std::vector<std::string>& get_vec2();
	std::vector<std::string>& get_vec3();
private:
	std::vector<std::string> vec0;
	std::vector<std::string> vec1;
	std::vector<std::string> vec2;
	std::vector<std::string> vec3;
};
Of course, the use of getters in this way is in and of itself a code smell, so a lot of people would just write:
 
class Game
{
public:
	std::vector<std::string> vec0;
	std::vector<std::string> vec1;
	std::vector<std::string> vec2;
	std::vector<std::string> vec3;

	Game();
	~Game();
	void postInput(/*some vectors or a tuple maybe lol*/);
	void update();
};
I suggest reviewing the way pointers and references work in C++ - and how vectors work, for that matter! - to understand why what you're doing is suboptimal. If you really must access several things at once that way, it's preferable to wrap them all up in their own structure and give each entry its own name so that calling code is less brittle. Edited by Oberon_Command

Share this post


Link to post
Share on other sites

Is the Single Responsibility Principle not idiomatic to C++? Why would anyone outside Game read/write directly from its members? Why would a graphics class need to know what method to call on a game class in order to "get" what needs to be drawn? I'm thinking that the graphics class and the game class shouldn't even know about eachother, they can be swapped out without affecting eachother. So I need to "take" from game, and pass it on to "graphics". And what is then the proper way to do just that?

 

With smart pointers, move semantics and other developments in the C++ standard, it seems to me that there is intent for the language to be used in a "pure" style and not to regress into C/Assembly. Such as "Game has data" and "graphics needs that data", lets return that data from game and give it to graphics. Straight from the horse's mouth: youtu.be/D5MEsboj9Fc?t=32m

 

The game data should have a handle and the handle should be returned. The tuple is the handle for this data... Is this not idiomatic to C++?

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this