Charlie
Charlie

Reputation: 41

Do C++ need additionnal code when inserting a pair in a map?

Hello great StackOverflow community ! I'm making some C++ and I meet a problem when inserting elements in a std::map.

Here are 2 maps, storing an ID-like unsigned int as key and another object as value :

    std::map<unsigned int, FIFO> _fifos;
    std::map<unsigned int, Kitchen> _kitchens;

Both maps are private in a class and I insert like this in a public method of that class:

    FIFO newFIFO(_internalCount);
    Kitchen newKitchen(_args, newFIFO);

    _kitchens.insert(std::make_pair(_internalCount, newKitchen));
    _fifos.insert(std::make_pair(_internalCount, newFIFO));

Here start the trouble.
Both my editor (VSCode) and compiler (g++) seems to accept the _fifos.insert() but not the _kitchens.insert().

VSCode tells :

no instance of overloaded function "std::map<_Key, _Tp, _Compare, _Alloc>::insert [with _Key=unsigned int, _Tp=Kitchen, _Compare=std::less<unsigned int>, _Alloc=std::allocator<std::pair<const unsigned int, Kitchen>>]" matches the argument list..."

While g++ first display this, after listing a bunch errors inside the depths of the C++ (notably in stl_pair.h):

error: no matching function for call to ‘std::pair<unsigned int, Kitchen>::pair(unsigned int&, Kitchen&)’
   66 |     _kitchens.insert(std::pair<unsigned int, Kitchen>(_internalCount, newKitchen));
      |                                                                                 ^


I already tried other std::pair definitions, like in this another question but without success.
Considering my poor knowledge of the C++'s depths, is there a type/syntax issue here or anything "missing" with the std::pair ?
Thanks in advance for your support !


EDIT:
Thanks for the suggestions and the link to the article on code examples/reproduction.
To add a little bit of context, this is a student project, aiming to learn and produce Concurrent Code.
The main idea is to create a pizzeria, where an object Reception represent the main process, Kitchen objects represent forked processes and Chefs tend to be objects managing a single, detached std::thread where abstract class Pizza will be cooked, for a certain amount of time.

I really don't want to bother you with software design, it's obvious there is some weird or bad choices. To give you a peek, each FIFO objects handle a system FIFO pipe, and they globally manage the IPC between the Reception and multiples Kitchens as the memory here is not shared.

Upon construction, Reception have it's own pipe (intended to receive confirmation from any Kitchens). This one is currently not implemented and certainly not very useful at the moment

Reception have to send orders to Kitchens, that's why Reception create a pipe and pass it to new Kitchens and both FIFO and kitchens are identified by an unsigned int _internalCount, incremented for every new Kitchens.
std::map<unsigned int, Kitchen> _kitchens will grow to store any new Kitchen created and std::map<unsigned int, FIFO> _fifo store each channel for the reception, and use them to send data.
Please note that the _internalCount is append to pipes names, which result of something like './pipes/kitchen_1', './pipes/kitchen_2', etc....
I don't know all the best pratices in StackOverflow to simplify and make it clear but below are the Reception, Kitchen and FIFO classes.

#include <fstream>
#include <map>
#include <queue>
#include <string>
#include <vector>

class FIFO {
public:
    FIFO() = delete;
    FIFO(int channel);
    FIFO(const FIFO& copy);
    ~FIFO();

public:
    void operator>>(std::string& container);
    void operator<<(const std::string& data);
    void operator<<(const char* data);
    FIFO& operator=(const FIFO& copy);
    void operator()();

public:
    std::string readFromChannel();
    void sendToChannel(const std::string& data);
    void flushChannel();

public:
    std::string getFIFOname() const;

private:
    void createDataChannel(int type);

private:
    std::string _fifoPath;
    std::fstream _fifo;
};

class Kitchen {
public:
    Kitchen() = delete;
    Kitchen(Args args, FIFO newFifo);
    ~Kitchen();

public:
    FIFO getFifo() const;
    bool getStatus() const;

private:
    void goCooking();
    void dispatchOrders();
    bool isFree();

private:
    Stock _stock;
    Time _time;

    int _maxChefs;
    std::vector<Chef> _chefs;

    std::queue<std::string> _orders;
    int _currentAssign;

    bool _status;
    bool _isOpen;

    FIFO _channel;
};

class Reception {
public:
    Reception() = delete;
    Reception(int argc, char** argv);
    ~Reception();

public:
    void openPlazza();
    void closePlazza();

public:
    void createKitchen(std::string order);
    void sendRequest(std::string order, int kitchenNb);
    void receiveConfirmation();
    unsigned int findKitchenAvailable();

private:
    bool _open; /*
OrderManager _checker;  ** Encapsulate other
Args _args;             ** aspects of the project
Shell _shell;           **
Process _process;       */

    FIFO _mainPipe; // Not very useful here
    std::map<unsigned int, FIFO> _fifos;
    std::map<unsigned int, Kitchen> _kitchens;
    unsigned int _internalCount;
};

int main() {}

Please let me know if you need something or if you want more details :)


EDIT 2:
I'm adding here some additionnal errors/note comming from g++ after the first line (display above in the initial question)

/usr/include/c++/9/bits/stl_pair.h:436:9: note: candidate: ‘template<class ... _Args1, long unsigned int ..._Indexes1, class ... _Args2, long unsigned int ..._Indexes2> std::pair<_T1, _T2>::pair(std::tuple<_Args1 ...>&, std::tuple<_Args2 ...>&, std::_Index_tuple<_Indexes1 ...>, std::_Index_tuple<_Indexes2 ...>)’
  436 |         pair(tuple<_Args1...>&, tuple<_Args2...>&,
/usr/include/c++/9/bits/stl_pair.h:436:9: note:   template argument deduction/substitution failed:
/usr/include/c++/9/bits/stl_pair.h:529:14: note:   mismatched types ‘std::tuple<_Tps ...>’ and ‘unsigned int’
  529 |       return __pair_type(std::forward<_T1>(__x), std::forward<_T2>(__y));

As always, thanks in advance for your time and your advices !


EDIT 3:
I starting to link everything in my brain now, thanks from you all. Things gets tricky there, but I definitely learning from all your suggestions. I'll closely read again the answer of @reinstate-monica and make further research by my side, I have the keys to resolve this and make it better.
Sorry for the lack of clarity, I didn't expected my first question to be as tricky and I wanted it clearer.
My next question will be clearer and I'll try to make minimal reproductible code right from the start, I'll closely look the good pratices of StackOverflow @ted-lyngmo!
Thanks everyone for your support and your time, take care of yourself !

Upvotes: 3

Views: 544

Answers (2)

Mooing Duck
Mooing Duck

Reputation: 66981

class Kitchen {
public:
    Kitchen() = delete;
    Kitchen(Args args, FIFO newFifo);
    ~Kitchen();

Since you defined a destructor, C++ is smart enough to realize the default copy and move constructors and assignments would almost certainly be wrong, so it doesn't generate defaults for you. As a result, it is impossible to make a copy of a Kitchen.

Kitchen newKitchen(_args, newFIFO);
_kitchens.insert(std::make_pair(_internalCount, newKitchen));

insert takes a pair, and tries to copy from that into the map. Without a copy constructor, it can't do that. There are two solutions. The obvious one is to make Kitchen copiable, but that may be tricky. The better solution is to not copy kitchen, but have the map create a new Kitchen in place as is demonstrated in Reinstate Monica's answer.

Upvotes: 2

First of all, you should be emplacing, not inserting - this avoids copies:

_kitchens.emplace(_internalCount, std::move(newKitchen));
_fifos.emplace(_internalCount, std::move(newFIFO));

Second of all, Kitchen is probably not copyable and not moveable, and therein lies your trouble. Ensure it can be copied or at least moved. If it is, you have to show a minimum example. I can write one but it will work, and a non-working variant would be trivial and not helpful. So show us your work first :)

Third of all: I don't know the design of your FIFO object, but passing its temporary instance to Kitchen in a constructor is likely nothing more than a bug. By the time the function where you do all this work exits, Kitchen will have a dangling reference. Thus what you really want is the following:

auto fifo_it = _fifos.emplace(std::piecewise_construct, {_internalCount}, {_internalCount}).first;
if (fifo_it.second)
  // if the new fifo was actually inserted
  _kitchens.emplace(std::piecewise_construct, {_internalCount}, {_args, fifo_it.first->second});

This way, the kitchen will have a reference to the fifo that at least has a flying chance at living long enough to be useful.

I'm extremely suspicious of the passing of _internalCount to both the map and the object contained in the map. Such duplication of information is a bad design smell, usually.

You should tell us what you're trying to achieve, and a better design may emerge.

Upvotes: 6

Related Questions