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

Started by
80 comments, last by Oberon_Command 7 years, 4 months ago

That is true. But you want to yank a resource away from an object and give that resource to another one. That is not an optimal approach. With the way you're doing it, you'll be creating and recreating the resource every frame. You don't have to do that.

Otherwise, I don't know what you want to achieve.

Look at this code, encapsulation is maintained, no reallocations, no pointers, no references, no consts, no nothing. But it kinda smells, and what if I want swap multiple vectors, I need to pack a tuple.


bool Application::processWindowMessages()
{
	// Temporary relinquish ownership of vector by passing it by rvalue to Window
	m_windowMessages = m_Window->swapMessages(std::move(m_windowMessages));

	for (auto& windowMessage : m_windowMessages)
	{
		UINT message = std::get<0>(windowMessage);
		WPARAM wParam = std::get<1>(windowMessage);
		LPARAM lParam = std::get<2>(windowMessage);

		switch (message)
		{
			//...
			// Handle messages
			//...
		}
	}
	
	return true
}

std::vector<std::tuple<UINT, WPARAM, LPARAM>>& Window::swapMessages(std::vector<std::tuple<UINT, WPARAM, LPARAM>>&& messages)
{
	std::swap(messages, m_messages);
	m_messages.resize(0);
	return messages;			// return by rvalue, giving back ownership
}
Advertisement

Well, that suggests to me std::swap is the wrong solution, isn't it?

I mean, everything else seems ok, so that must be the wrong thing.

The answer thus might be "don't swap data out and in".

One step further, don't give ownership of that data to the object at all. Just give it a pointer to the data.

Obviously, that creates the problem that you need storage for it somewhere. The simple form is to store that data in an object that produces it, and give others read access with a reference or so. Another option is to make a separate object that does nothing, except holding the data, and pass a reference to that around. Apparently this is not good either.

In the end, it seems your constraints are too tight. No solution has been proposed that solves all problems, so it may not exist. To get forward, you'll have to pick the "least worse" solution, instead of the one "good in every way" solution.

The above code solve for encapsulation as nothing will hold a reference or pointer to private members. So I can guarantee no dangling pointers or references. I avoid reallocations by moving the vector in and out. The only problem I can think of with that code is a caller might move in a vector that is already gutted from a previous move. I would love to give what I have now a cosmetic makeover and extend it for multiple vectors.

Look at this code, encapsulation is maintained, no reallocations, no pointers, no references, no consts, no nothing.


Could you tell us, in your own words, what you think encapsulation means? You didn't answer the question I posed to you in my last post.

Unrelated, but you seem really intent on using a tuple there and I don't see what it buys you. Win32 already comes with a structure that stores those exact fields. Why not use that? Or create your own custom structure if MSG is overkill for you?

what you think encapsulation means?

wiki - Encapsulation is the hiding of information to ensure that data structures and operators are used as intended

Unrelated, but you seem really intent on using a tuple there and I don't see what it buys you

Tuple is faster than a default struct http://stackoverflow.com/a/40308910, I can take out members that I don't need from MSG, it has move semantics from STL, and I can inline the tuple layout in code.

wiki - Encapsulation is the hiding of information to ensure that data structures and operators are used as intended


Key point there - ensure that data structures and operators are used as intended. Remind me, again, does the following break encapsulation?

class Thing {
public:
   Thing() : m_str("This is a thing") {} 

   const std::string& getStr() const {
     return m_str;
   }

private:
  std::string m_str;
};
The internal array that std::vector uses as storage is a private member variable on std::vector. Does calling std::vector::at() break encapsulation?

Tuple is faster than a default struct


What? That doesn't make any sense. A tuple under the hood is just a struct constructed with recursive inheritance. I suspect that benchmark is leaving out some important details - std::tuple may be handling operator< and operator== in a smarter way than the benchmark's struct, for instance. There's no reason you couldn't do the same thing with a structure, and the benchmark even says this. Your link doesn't pass the smell test for me.

The benchmark also only deals with sorting. Are you sorting your messages? If not, then that benchmark does not apply to you. Even if tuple WAS faster in that case, that doesn't mean that it's faster for everything.

StackOverflow has some useful information on it, but it's not an authority on any subject. Don't treat it like one. Some "answers" on there are actually total bullshit.

it has move semantics from STL,


A bare struct will have a move constructor defined for it by the compiler. There is no way that this is slower than the one for std::tuple. And anyway, I could be wrong on this point but I believe the elements in your vector won't get moved when the vector itself is moved.

and I can inline the tuple layout in code.


And then repeat it, everywhere that you use the tuple. That's not an advantage. That's a disadvantage - now everything that uses the tuple has to ensure that the tuple type declarations match. That's more work for you.

edit: Yeah, that benchmark is clearly bullshit. Look at the guy's "default struct" implementation of operator== - it's constructing two tuples every time it's called, then comparing them! Of course, that's going to be slower... Plus the operator< logic in the "default struct" is more complex than it needs to be, as far as I can tell.

Posted Today, 05:38 PM Quote wiki - Encapsulation is the hiding of information to ensure that data structures and operators are used as intended Key point there - ensure that data structures and operators are used as intended. Remind me, again, does the following break encapsulation? class Thing { public: Thing() : m_str("This is a thing") {} const std::string& getStr() const { return m_str; } private: std::string m_str; };The internal array that std::vector uses as storage is a private member variable on std::vector. Does calling std::vector::at() break encapsulation?

Yes?


std::unique_ptr<Thing> p_thingy = std::make_unique<Thing>();

auto refString = thingy->getStr();

//..
// p_thingy is destroyed
//..

std::cout << refString.at(i)	// oopsie, thingy no longer exist!

#include <iostream>
#include <string>
#include <tuple>
#include <vector>
#include <random>
#include <chrono>
#include <algorithm>

class Timer {
public:
  Timer() { reset(); }
  void reset() { start = now(); }

  double getElapsedSeconds() {
    std::chrono::duration<double> seconds = now() - start;
    return seconds.count();
  }

private:
  static std::chrono::time_point<std::chrono::high_resolution_clock> now() {
    return std::chrono::high_resolution_clock::now();
  }

  std::chrono::time_point<std::chrono::high_resolution_clock> start;

};

struct Smart {
  int X;
  int Y;
  double Cost;
  std::string Label;

  bool operator==(const Smart &rhs) {
    return
      (X == rhs.X) &&
      (Y == rhs.Y) &&
      (Cost == rhs.Cost) &&
      (Label == rhs.Label);
  }

  bool operator<(const Smart &rhs) {
    if(X > rhs.X) { return false; }
    if(Y > rhs.Y) { return false; }
    if(Cost > rhs.Cost) { return false; }
    if(Label >= rhs.Label) { return false; }
    return true;
  }
};

using Dumb = std::tuple<int, int, double, std::string>;

std::pair<std::vector<Smart>, std::vector<Dumb>> generate() {
  std::mt19937 mt(std::random_device{}());
  std::uniform_int_distribution<int> dist;

  constexpr size_t SZ = 1000000;

  std::pair<std::vector<Smart>, std::vector<Dumb>> p;
  auto& s = p.first;
  auto& d = p.second;
  s.reserve(SZ);
  d.reserve(SZ);

  for(size_t i = 0; i < SZ; i++) {
    s.emplace_back();
    auto& sb = s.back();
    sb.X = dist(mt);
    sb.Y = dist(mt);
    sb.Cost = sb.X * sb.Y;
    sb.Label = std::to_string(sb.Cost);

    d.emplace_back(std::tie(sb.X, sb.Y, sb.Cost, sb.Label));
  }

  return p;
}

int main() {
  Timer timer;

  auto p = generate();
  auto& structs = p.first;
  auto& tuples = p.second;

  timer.reset();
  std::sort(structs.begin(), structs.end());
  double stSecs = timer.getElapsedSeconds();

  timer.reset();
  std::sort(tuples.begin(), tuples.end());
  double tpSecs = timer.getElapsedSeconds();

  std::cout << "Structs took " << stSecs << " seconds.\nTuples took " << tpSecs << " seconds.\n";

  std::cin.get();
}

Structs took 0.0814905 seconds.
Tuples took 0.282463 seconds.
void hurrrrrrrr() {__asm sub [ebp+4],5;}

There are ten kinds of people in this world: those who understand binary and those who don't.

Release mode using what compiler and optimization flags?

Yes?


That example doesn't do what you think it does. refString will be a copy of the string in the class, not a reference. You have to explicitly go "auto&" to make a variable declared with type inference a reference.

Even if we change that to auto& so the example you posted does what you think it does, I would say that you're confusing ownership semantics and encapsulation. Encapsulation is about making it so that a user of the class doesn't need to know how the class's internals work to use it. But in C++, it's generally assumed that if a object owns some data, then the data will disappear when the object does, unless it's passed out in a way that prevents that from happening, and anyway the encapsulation is of the object's internal state and not necessarily the data it contains.

Again, it looks to me like you're being more dogmatic than is warranted.

Release mode using what compiler and optimization flags?


You could always run it yourself, you know. ;)

This topic is closed to new replies.

Advertisement