CODEWITHSUNDEEP
c++error-handlingcompiler-warnings
Byron
Byron

Reputation: 231

Not All Control Paths Return a Value? Warning

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!

Upvotes: 4

Views: 5016

Answers (4)

Andy Prowl
Andy Prowl

Reputation: 126452

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.

Upvotes: 19

CJ McAllister
CJ McAllister

Reputation: 331

I would recommend using brackets with all of your if statements, even with one-line bodies. They are not strictly necessary, and you can write perfectly legal code without them, but they make your code much more readable, and errors such as this will be much easier to find.

Additionally, it seems you have a fundamental misunderstanding of how exceptions work. If your code were to encounter an exception, it would jump straight to the catch block, and not execute any subsequent code in the try block. Therefore, the return statement in you try block will never be reached, and your function will not return anything, since the catch blocks lack return statements.

You can resolve this issue by adding a return statement in your catch block.

Upvotes: 0

Joseph Mansfield
Joseph Mansfield

Reputation: 110668

When the exception is thrown, it will be caught by one of the two catch statements. However, they still need to return a value from the function. You could place a return statement right at the end of your function.

However, if Pop throws an exception because the Stack is empty, it makes much more sense for it to let the exception propagate out of the function. Why is Pop itself attempting to handle the exceptional situation?

Upvotes: 1

alexrider
alexrider

Reputation: 4463

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

Upvotes: 1

Related Questions