Lightning talk #1 – Random bugs

This is a cross-post from www.badearobert.ro.

In this lightning talk i'm gonna write about some errors that are not so easily detected.

The idea and the code is copied from this talk.

1. Const correct-ness

void Widget::configure(const map<string, string> & settings) 
{
   // try to read from
   m_timeout = settings["timeout"];
   m_size = setting["max_items"];
}

What would be the problem with this code? It doesn't compile, of course.

error message: passing const map... as 'this' argument discards qualifiers

Why would this happen ? That's because the brackets operator [] from a map is reading OR setting the value if it doesn't exist in the map. This implies that the map is modified after such operation, so it cannot be const.

A common fix bug would be to remove the const

void Widget::configure(map<string, string> & settings) 
{
   // try to read from
   m_timeout = settings["timeout"];
   m_size = settings["max_items"];
}

This will allow us to kick ourselves in the foot, because if the key is not found in the map, or whether we wrote it incorrectly, it will actually update our map instead.

2. Returning reference to temporary object

string get(const map<string,string>& map, const string& key, const string& default) 
{
   auto pos = map.find(key);
   return (pos != map.end() ? pos->second : dflt);
}

This is a function that returns the value from a map, or the default value otherwise.

The problem with this code is that we have an extra copy each time we call this function.

A common fix bug would be to make the return value const ref.

const string& get(const map<string,string>& map, const string& key, const string& default) 
{
   auto pos = map.find(key);
   return (pos != map.end() ? pos->second : default);
}

Now we don't have an extra copy, if the key is found in the map. Otherwise, we will return the default parameter sent with the function.

The problem here appears for the following code

auto& value = get(myMap, key, "127.0.0.1"); // default temporary.

value will now be a reference to a temporary that is no longer there.

A solution would be to create the string one line about, instead of creating it in-place.

3. Unnamed variables

Does this compiles?

std::string(str);

The problem is that it does. => the same as int(0);

It's a declaration of a string. => the same as std::string str;

What about this line?

std::string("wow");

It's a copy constructor (Creating a temporary string). => the same as std::string str("wow");

This is not really that faulty, but could be the cause of hidden bugs in places where RAII is involved, in order to lock a mutex or a critical section.

void execute() 
{
   unique_lock<mutex>(m_mutex);
   do_execute();
}

Unique_lock is affected because it allows a default constructor. Lock_guard does not have this problem.

If we use static code analysis (with the proper flags), this would be caught because it actually shadows the member variable (by creating a local one, instead of using it)