Not All Control Paths Return a Value? Warning

Question

Ive got a short bit of code that is giving me the following the warning upon compilation:

'BGOLUB::Containers::Stack::Pop' : not all control paths return a value

Here is the code:

template<typename T>
T Stack<T>::Pop()                                                                           
{

try
{
    if (m_index<0) throw OutOfBoundsException(m_index);

    --m_index;
    return(m_array[m_index]);
}

catch(OutOfBoundsException&)
{
    cerr<<"Underflow Index = "<<m_index<<endl;
}

catch(...)
{
    cerr<<"Unhandled Error Occured"<<endl;
}
}

Any advice?

Many thanks!

Answer 1

Any advice?

The compiler is giving you the best advice. Not all the control paths in your function contain a return statement, and your function is supposed to return a value.

If an exception is thrown and control is transferred to a catch handler, that handler will print something to cerr and then flow off the end of your function without actually returning anything.

This is Undefined Behavior. Per Paragraph 6.6.3/2 of the C++11 Standard:

[..] Flowing off the end of a function is equivalent to a return with no value; this results in undefined behavior in a value-returning function.

For default-constructible values, you could fix this by adding a return T() statement right before the end of your function:

template<typename T>
T Stack<T>::Pop()
{
    try
    {
        // ...
    }
    catch (OutOfBoundsException&)
    {
        // ...
    }
    catch (...)
    {
        // ...
    }

    return T();
//  ^^^^^^^^^^^
}

A more reasonable approach is, however, to not have Pop() swallow the exception, but rather re-throw it. Pop() does not have strategic-level information on how to recover from an error occurred in this context:

template<typename T>
T Stack<T>::Pop()
{
    try
    {
        // ...
    }
    catch (OutOfBoundsException&)
    {
        // ...
        throw; // <== Re-throw after printing the diagnostic
    }
    catch (...)
    {
        // ...
        throw; // <== Re-throw after printing the diagnostic
    }
}

Even better would be if the responsibility for logging an error message did not belong to Pop() at all, since Pop() is likely supposed to be re-used by code with different requirements in this sense (some may not want to log anything, some may want to log messages to a file, some may want to log messages in a different language, and so on).

So a more reasonable version of your function would actually be:

template<typename T>
T Stack<T>::Pop()                
{
    if (m_index<0) throw OutOfBoundsException(m_index);
    --m_index;
    return(m_array[m_index]);
}

In general, you should try (no pun intended) to avoid try/catch blocks unless you have to:

• Translate an exception
• Recover from the error (but you need strategic knowledge for doing this)

If this not your task (as is the case for a function like Pop() above), in most situations the best thing to do is to not handle exceptions at all and let them propagate up the call stack.

To quote Dave Abrahams:

Often the best way to deal with exceptions is to not handle them at all. If you can let them pass through your code and allow destructors to handle cleanup, your code will be cleaner.

To avoid leaking memory, resources, or in general responsibilities, write code that is exception-safe by using adequate RAII wrappers. Excellent guidelines in this sense are given in this two-part talk by Jon Kalb.

In particular, avoid writing catch (...) handlers: exceptions were invented to prevent programmers from ignoring errors, and swallowing them all in a universal handler without re-throwing them is the best way to ignore them.

---

NOTE:

Notice, that your implementation of Pop() is a bit problematic: what happens if the copy constructor or move constructor of T throws when returning the element back to the caller, after you already modified the stack pointer?

This is the reason why the C++ Standard Library defines two separate functions pop() and top(): because it allows to offer strong guarantee, i.e. to give transactional semantics to your pop() operation - either the element is removed without exceptions being thrown, or the function had no effect at all.

Answer 2

You need either rethrow exceptions, or return something probably T() at the end of function.