I've spent a bit of time reviewing someone else's code recently and I've come to an impasse with them, so they have a lot of code which will take some standard container, and the code doesn't just initialise the local copy from the passed in reference... No it'll iterate over the list of elements adding them to the class version.
I have picked fault with this, it's not RAII, it looks ugly and if you're threading you can create your class instance and the member is empty or in a partially filled state before the loop within the constructor is finished... I highlight this in red below...
My solution? Just initialise the member from the reference - see the green highlight in the code below.
My results from the timing below?
These times are "microseconds" so tiny... But with just constructing from the existing reference we always get a lower time, quicker code...
Running this test 30,000 times, trying it in different orders and with maps of upto 1000 elements I had a rough average increase of 60% speed by using the copy constructor of std::map rather than reallocating new memory for each pre-existing element.
I wanted therefore to understand the reasoning behind the original code, was there some reason to perform a clone (alloc and assign new instance) operation for each pair in the map being passed in? Looking at the code there was no apparent reason, so I spoke to the developer, asking why they had performed the construction in this manner... Their reply...
"That's how you initialise a container"
You can load up a container in this manner, but you already have the contents of the map, you're just copying it... "Why don't you use the copy constructor?"... I asked...
"I didn't write one"
Hmm, "You don't, the compiler generates it for you, std::map has its own copy constructor". Use the copy constructor folks, trust me.
#include <map>
#include <chrono>
#include <string>
#include <iostream>
using Mapping = std::map<int, int>;
class A
{
private:
Mapping m_TheMapping;
public:
A() = delete;
A(const A&) = delete;
void operator=(const A&) = delete;
A(const Mapping& p_Mapping)
:
m_TheMapping(p_Mapping)
{
}
A(const Mapping& p_Mapping, const bool& p_Other)
:
m_TheMapping()
{
for (auto i : p_Mapping)
{
m_TheMapping.emplace(i);
}
}
inline Mapping& Mapping()
{
return m_TheMapping;
}
};
int main()
{
Mapping l_m
{
{ 0, 0 },
{ 1, 1 },
{ 2, 2 },
{ 3, 3 }
};
auto l_time(std::chrono::high_resolution_clock::now());
// Copy Construction
A l_map(l_m);
auto l_timeB(std::chrono::high_resolution_clock::now());
auto l_Dur(l_timeB - l_time);
std::cout << std::chrono::duration_cast<std::chrono::microseconds>(l_Dur).count() << std::endl;
auto l_time2(std::chrono::high_resolution_clock::now());
// Clone Construction
A l_map2(l_m, true);
auto l_time2B(std::chrono::high_resolution_clock::now());
auto l_DurB(l_time2B - l_time2);
std::cout << std::chrono::duration_cast<std::chrono::microseconds>(l_DurB).count() << std::endl;
}
