3

I am writing a template function which accepts a custom class (that can be any class or primitive type) as a template argument, then reads some data (of that type) from an input stream, and then stores it an unordered map similar to this one:

std::unordered_map<CustomClass, std::vector<CustomClass>> map;

I have implemented a custom class to test the behavior. I have overloaded the std::hash so that this class can be stored in an unordered map as a key and overloaded all operators and constructors such that whenever they are called, I get a message in the console (example, when a copy constructor is called, I get a message "copy constructor [..data...]")

EDIT: As requested in the comments, here is the custom class definition and implementation (please note: the class here is only a placeholder so we can discuss the general idea behind this question. I am well aware that it is dumb and should not be implemented like this. The code for operators >> and << is not here, to avoid clutter)

class CustomClass {
public:
    CustomClass(int a=0) {
        std::cout << "default constructor" << std::endl;
        m_data = a;
    }

    CustomClass(const CustomClass& other) {
        std::cout << "copy constructor "  ;//<< std::endl;
        m_data = other.m_data;
        std::cout << "[" << m_data << "]"  << std::endl;
    }

    CustomClass(CustomClass&& other) {
        std::cout << "move cosntructor" << std::endl;
        m_data = other.m_data;
    }

    CustomClass& operator=(const CustomClass& other) {
        std::cout << "copy assignment operator" << std::endl;
        if(this != &other){
           m_data = other.m_data;
        }
        return *this;
    }

    CustomClass& operator=(CustomClass&& other) {
        std::cout << "move assignment operator" << std::endl;
        if(this != &other){
            m_data = other.m_data;
        }
        return *this;
    }

    ~CustomClass() {
        std::cout << "destructor" << std::endl;
    }

    int m_data;
};

Now my question is this: Is it possible to read data from the input stream and construct it inplace where it is needed without a copy constructor call?

Example of some code:

CustomClass x1;                        // default constructor call
CustomClass x2;                        // default constructor call
std::cout << "----" << std::endl;
std::cin >> x1 >> x2;                  // my input
std::cout << "----" << std::endl;
map[x1].emplace_back(x2);              // 2 copy constructor calls
std::cout << "----" << std::endl;
std::cout << map[x1][0] << std::endl;  // operator==  call
std::cout << "----" << std::endl;

And here is an example output from that code:

default constructor
default constructor
----
[1]
[2]
----
copy constructor [1] 
copy constructor [2]
----
operator ==
[2]
----
destructor
destructor
destructor
destructor

I would like to have it so that every object of this class is constructed only once.

Is it possible to avoid these copy constructors? If not both, then at least the one that is called during the emplace_back() call? Is it possible to construct the object in the vector exactly where it needs to be in memory but that this sort of call works for every type?

If I need to further elaborate on my question please tell me in the comments, I will be happy to do so

marc_s
  • 732,580
  • 175
  • 1,330
  • 1,459
  • Assuming this is C++11 or later, it looks like you're breaking [the rule of five](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c21-if-you-define-or-delete-any-copy-move-or-destructor-function-define-or-delete-them-all). Are you familiar with [move semantics](https://stackoverflow.com/q/3106110/11082165)? – Brian61354270 Dec 01 '21 at 23:17
  • @Brian I am using C++17. I have overloaded the move constructor and move = operator as well (however they are not beeing called here). I am fammiliar with move semantics, but not very deeply. I know move semantics should be used when pointers are in play, however that is not the case with my code, the CustomClass only stores 3 integer variables. Would you care to elaborate what i am doing wrong here? – TheMemeMachine Dec 01 '21 at 23:25
  • You can call `resize()` and use `back()` to read it from stream. – Dmitry Kuzminov Dec 01 '21 at 23:26
  • 2
    On a second look, the issue I thought I saw isn't there. I think the only issue is that you're not communicating that `x2` can expire after `map[x1].emplace_back(x2);`, so a copy needs to be made in order to bind with the rvalue reference that [emplace_back](https://en.cppreference.com/w/cpp/container/vector/emplace_back) excepts. You can fix that by using `map[x1].emplace_back(std::move(x2));`. See also [What is std::move(), and when should it be used?](https://stackoverflow.com/q/3413470/11082165) – Brian61354270 Dec 01 '21 at 23:28
  • you can use [inplacer](https://stackoverflow.com/questions/45345175/avoiding-extra-move-in-make-unique-make-shared-emplace-etc-for-structures-that-u) to construct objects inside of containers. You can make a family of `template T read(istream&)` functions to avoid unnecessary default ctor call. In general, if you care about performance and clarity, I suggest staying away from iostreams. – C.M. Dec 01 '21 at 23:32
  • @DmitryKuzminov I am not sure I understand. Could you elaborate? – TheMemeMachine Dec 01 '21 at 23:35
  • @C.M. Could you link me some reading material for that? I have no idea how to do that and what that actaully means – TheMemeMachine Dec 01 '21 at 23:37
  • Since you said that `CustomClass` only stores 3 integers, it may be advisable to follow the [rule of zero](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-zero) here. It doesn't sound you need any custom logic to manage resources, so defining any of the default operations probably isn't necessary. Plus, **not** defining default operations yourself can let your class satisfy a number of named requirements with their own potential benefits. See [What are Aggregates and PODs and how/why are they special?](https://stackoverflow.com/q/4178175/) for an extensive discussion of that. – Brian61354270 Dec 01 '21 at 23:39
  • @Brian if i just add that std::move() my example program crashes when the move constructor is called. Any prerequisites to std::move i should be aware of? – TheMemeMachine Dec 01 '21 at 23:42
  • @TheMemeMachine None really, there's probably an issue with your move constructor's implementation. Could you add the definition of `CustomClass` with an [edit]? – Brian61354270 Dec 01 '21 at 23:45
  • @Brian Well for this class sure, but as i stated at the begining of my question, this whole logic is supposed to be implemented in a function that can accept ANY type of data, which may not be POD. Also, the reason I am implementing these constructors myself here is to track when they are called (only in this example, untill i reach a general solution), so not using them here is not serving me any purpose – TheMemeMachine Dec 01 '21 at 23:46
  • @Brian Sure, I will first edit the whole class (and the post itself) to only use 1 integer, so there is less code, but everything else will remain the same. Give me a minute – TheMemeMachine Dec 01 '21 at 23:50
  • On second (third?) look, I think you actually want to be using [`vector::push_back`](https://en.cppreference.com/w/cpp/container/vector/push_back), as in `map[x1].push_back(std::move(x2));`. `emplace_back` is for performing the actual construction in-place, but you already have a fully constructed object. – Brian61354270 Dec 01 '21 at 23:51
  • @Brian I have edited. Also, the `map[x1].push_back(std::move(x2));` also produces a crash at the move constructor – TheMemeMachine Dec 02 '21 at 00:02
  • @TheMemeMachine Omitting the interaction with iostream and std::unordered_map, your code [works fine](https://godbolt.org/z/1vh34zzTa) for me. If I had to guess, the issue is with how your implementing hashing for std::unordered_map. – Brian61354270 Dec 02 '21 at 00:18
  • @Brian Hey, so added the rest of the functions to [godbolt](https://godbolt.org/z/K1fvrsz3z) so you can see. The hashing function works just fine. The code does seem to work here for some reason, but not on my machine. I am compiling with MSVC with /std:c++17 without any optimisations. Also, as you can see in godbolt code, even though the move constructor is called, there are 4 destructors, so something does not seem to be working correctly with the std::move approach – TheMemeMachine Dec 02 '21 at 09:44
  • 1
    @TheMemeMachine I think the four destructor calls are expected: one for `x1`, one for `x2`, one for the key in the map, and one for the value in the map. The moved-from objects still have their destructors called. I'm not sure why your program is crashing when compiled with MSVC given that it works with both GCC and clang. – Brian61354270 Dec 02 '21 at 16:14

2 Answers2

0

So you have an std::vector and you want to put an element there avoiding unnecessary copying. Assuming that move constructor for your class is cheap, the first option is to define a non-trivial constructor, read the parameters from stream and then emplace_back a newly created object:

using CustomClass = std::vector<int>;

std::vector<CustomClass> v;
size_t size;
int value;
std::cin >> size >> value;
v.emplace_back(size, value);

Here I defined the CustomClass as a vector of integers, and it has a constructor that takes 2 parameters: size and the value. For sure it is cheaper to read there two integers and create the instance of CustomClass only once and using the emplace_back for this purpose, rather than to create the instance and copy it using push_back:

using CustomClass = std::vector<int>;

std::vector<CustomClass> v;
size_t size;
int value;
std::cin >> size >> value;
CustomClass instance(size, value);
v.push_back(instance);

This however doesn't give you much benefits to compare with pushing back the r-value:

using CustomClass = std::vector<int>;

std::vector<CustomClass> v;
size_t size;
int value;
std::cin >> size >> value;
v.push_back(CustomClass(size, value));

Anyway, you need to keep in mind that both push_back and emplace_back may require reallocation of the elements, and that may be inefficient, especially if your CustomClass has no no-throwing move constructor.

Another problem could be if your class doesn't have a reasonable constructor (or the size of the values you need to pass to the constructor are almost the size of the object). In this case I'm offering you the solution to resize() and read to the back()

If the reallocation is something that you are not afraid of (for example you know the number of elements ahead of time and reserve the buffer), you can do the following:

std::vector<CustomClass> v;

v.resize(v.size() + 1);
std::cin >> v.back();

In this case you create a default value once and then read the contents.

Another solution could be to pass the std::istream to the constructor of CustomClass:

class CustomClass {
public:
    CustomClass(std::istream&);
};

std::vector<CustomClass> v;
v.emplace_back(cin);

Update:

Assuming that you know nothing about the actual type of the CustomClass, the most generic (not fully generic, as it still requires default constructor to be able to be pushed with resize()) is to use resize()/back() idiom.

Dmitry Kuzminov
  • 6,180
  • 6
  • 18
  • 40
0

This is how you do it (avoids any unnecessary ctor calls, including a default one):

#include <vector>
#include <unordered_map>
#include <cstdio>
#include <iostream>


using namespace std;


//--------------------------------------------------------------------------
template <class F> struct inplacer
{
    F f;
    operator invoke_result_t<F&>() { return f(); }
};

template <class F> inplacer(F) -> inplacer<F>;


//--------------------------------------------------------------------------
struct S
{
    S(istream&) { printf("istream ctor\n" ); }
    S()         { printf("ctor\n" ); }
    ~S()        { printf("dtor\n" ); }
    S(S const&) { printf("cctor\n"); }
    S(S&&)      { printf("mctor\n"); }
    S& operator=(S const&) { printf("cop=\n"); return *this; }
    S& operator=(S&&)      { printf("mop=\n"); return *this; }
    friend bool operator==(S const& l, S const& r) { return &l == &r; } //!! naturally, this needs proper implementation
};

template<> struct std::hash<S>
{
    size_t operator()(S const&) const noexcept { return 0; } //!! naturally, this needs proper implementation
};


//--------------------------------------------------------------------------
template<class R> struct read_impl;   // "enables" partial specialization

template<class R> R read(istream& is)
{
    return read_impl<R>::call(is);
}

template<> struct read_impl<S>
{
    static auto call(istream& is) { return S(is); }
};

template<class T> struct read_impl<vector<T>>
{
    static auto call(istream& is)
    {
        vector<T> r; r.reserve(2);          //!! naturally you'd read smth like length from 'is'
        for(int i = 0; i < 2; ++i)
            r.emplace_back(inplacer{[&]{ return read<T>(is); }});
        return r;
    }
};

template<class K, class V> struct read_impl<unordered_map<K, V>>
{
    static auto call(istream& is)
    {
        unordered_map<K, V> r;
        r.emplace( inplacer{[&]{ return read<K>(is); }}, inplacer{[&]{ return read<V>(is); }} );
        return r;
    }
};


//--------------------------------------------------------------------------
auto m = read<unordered_map<S, vector<S>>>(cin);

As you can see in the output -- you end up with 3 "istream ctor" calls and 3 "dtor" calls.

As for iostreams -- stay away from them if you care about performance, clarity, etc... The most ridiculous library ever.

P.S. "Partial specialization for function templates" trick is stolen from here.

C.M.
  • 3,071
  • 1
  • 14
  • 33
  • Well "3 istream ctor calls" isn't correct, because there are only two objects in the stream. In fact there is no way to avoid reading the key into a temporary object, because you can't construct the key in-place until you know you are adding a new entry in the map (vs appending to the vector of an existing map entry), and you can't know you are adding until after the object has been read from the stream. – Ben Voigt Dec 02 '21 at 23:15
  • @BenVoigt if you read that code carefully, you'll notice we create 3 instances -- 1 for key and 2 -- for vector – C.M. Dec 02 '21 at 23:20
  • OP's code only extracts key/value pairs from the istream. Any "optimization" of that that reads 3 objects from the istream is incorrect, because the stream doesn't contain an odd number. Also note that in OP's code, `map[x1]` may already exist, rather than adding a new entry. I think your code lost that ability... – Ben Voigt Dec 02 '21 at 23:22
  • @BenVoigt It is trivial to change my code to do whatever OP needs. It is just a demo on how to extract data from stream without unnecessary (ctor) calls. Btw, afaik, `map::emplace()` will end up generating entire node before using it's key in a lookup (followed by discard, if key already exists). – C.M. Dec 02 '21 at 23:28