appropriate way to deal with compiler warning non-void function doesn't return value in all paths

[mac10warrior]

Sorry if this has been asked, I didn't find it. Working through Bjarne's PPP and using g++ on my mac. Total beginner.
99% sure I get the warning because my default case calls error() and doesn't return a double. Is there a standard way to do this? A lot of commentary I've seen regarding this warning is "treat it like an error."

This is code parsing input from command line for a calculator.

double primary()
{
	Token t = ts.get();
	switch (t.kind) {
	case '(':
	{	double d = expression();
	t = ts.get();
	if (t.kind != ')') error("'(' expected");
	}
	case '-':
		return -primary();
	case '+':
		return primary();
	case number:
		return t.value;
	case name:
		return get_value(t.name);
	default:
		error("primary expected");
		// return 0; // actually works, but seems ugly
	}
}

[dutch]

Why not return 0? How is that "ugly"?
Are you saying that error doesn't return (it calls exit()) ?
In that case, you should still have the return, followed by the comment "never reached".
It's always best to silence warnings.

[mbozzi]

If error does not return it should be declared noreturn, e.g.:
[[noreturn]] void error(char const* s) { throw std::runtime_error{s}; }

[Ganado]

I remember reading an article a while back about how having superfluous logic in switches in functions can actually limit the amount of optimization that a compiler will do. The cool thing about undefined behavior is that a compiler can just assume that it doesn't happen, at the cost of code maintainability or future bugs if someone tries to extend your project without knowing what they're doing. And 99% chance it's premature optimization, so overall, I don't suggest having a path that doesn't return a value, especially not without profiling.

If it is actually logically impossible for a path to reach that part of the code, personally I'd put an assert there and return a dummy value. And make sure you have unit tests to make sure bad input being parsed doesn't reach that point.

[jonnin]

I don't think there would be any extra logic here, though: it already had to handle the default case, you are adding another statement, not more logic (read:branching/lookups). Its the number of cases and fall throughs and how those behave that can bungle the optimizations, if I recall the issue.

I do not care for asserts: they behave differently in release vs debug code. I love the idea though: if you get here, stop cold (patch a path back to a graceful exit with an error report, in some fashion).

I am not a purist. If you KNOW it can't reach the logic, then ignore the warning.

bool tf(int x)
{
if(x > 10) return true;
else return false;
// it cannot get here. No matter what you do, it cannot happen.
//putting bogus code here to silence a warning serves no purpose. now you have to
//explain to the reader that the code you put in can't fire.
}

this one of course can be resolved by removing the else. Often a re-write will solve the issue.
and naturally the problem is that such cases where you KNOW for sure every possible response is already covered are rare.

another idea if return 0.0 (a valid value!) does not work for you, you can return one of the NAN double codes.

[JLBorges]

mbozzi> If error does not return it should be declared noreturn

+1

This would tell the compiler that normal flow of control does not apply after a call to this function; it would let the compiler (and static analysers like the Clang static analyser) avoid spurious warnings about return values and the like.

[seeplus]

error() as defined by PPP as simply:

inline void error(const std::string& s)
{
	throw std::runtime_error(s);
}

It's part of the std_lib_facilities.h file provided. So it doesn't return, so any statements placed after error() are not executed.

primary() should be called from within a try/catch but PPP hasn't covered that at this point in the book.

In this case, it's the exception (pun. groan!) to ignore the warning.

Also see mbozzi's post re marking error as noreturn. Bjarne should have coded his error functions as [[noreturn]]

[Handy Andy]

Hello mac10warrior,

All very nice answers, but missing the problem.

This has nothing to do with the "default case", but with the function as a whole. Your function double primary() promises to return a value, but does not in all paths.

The case statements do return valued, but after the "default case" you leave the switch and return nothing.

Try this:

double primary()
{
	Token t = ts.get();
	switch (t.kind) {
	case '(':
	{	double d = expression();
	t = ts.get();
	if (t.kind != ')') error("'(' expected");
	}
	case '-':
		return -primary();
	case '+':
		return primary();
	case number:
		return t.value;
	case name:
		return get_value(t.name);
	default:
		error("primary expected");
		// return 0; // actually works, but seems ugly
	}

        return 0.0;  // <--- Added.
}

You do not have to return "0.0" it could be any value that you may need to use as a check that the function worked.

When calling the function you might be able to use something like:

if (!double primary())  // <--- Meaning if the returned value is "0.0" do something. 

Andy

[seeplus]

adding return 0.0 at the end is the same as adding return 0 in default (which as the OP says, works) - but it will never get executed.

There's also a problem that d is assigned a value from expression() but never used. The '(' case 'drops' to the '-' case if there's no error. Another way would be to only return a value at the end of the function. Something like (not tried):

double primary()
{
	double value {NAN};
	Token t {ts.get()};

	switch (t.kind) {
		case '(':
			value = expression();
			t = ts.get();
			if (t.kind != ')')
				error("'(' expected");

			break;

		case '-':
			value = -primary();
			break;

		case '+':
			value = primary();
			break;

		case number:
			value =  t.value;
			break;

		case name:
			value = get_value(t.name);
			break;

		default:
			error("primary expected");
	}

	return value;
}

and potentially something similar for the other functions expression() et al.

[JLBorges]

> adding return 0.0 at the end is the same as adding return 0 in default (which as the OP says, works)

Yes, there is no difference; in either case the compiler can see that the return is always executed.

If error() is marked as [[noreturn]], adding a needless return 0.0 will result in a warning about unreachable code.

[seeplus]

What's with all the reporting? The OP post has also been erased. Ahh.......

[Handy Andy]

It comes at the end, but in case anyone is interested.

mac10warrior wrote:

Sorry if this has been asked, I didn't find it. Working through Bjarne's PPP and using g++ on my mac. Total beginner.
99% sure I get the warning because my default case calls error() and doesn't return a double. Is there a standard way to do this? A lot of commentary I've seen regarding this warning is "treat it like an error."

This is code parsing input from command line for a calculator.

double primary()
{
	Token t = ts.get();
	switch (t.kind) {
	case '(':
	{	double d = expression();
	t = ts.get();
	if (t.kind != ')') error("'(' expected");
	}
	case '-':
		return -primary();
	case '+':
		return primary();
	case number:
		return t.value;
	case name:
		return get_value(t.name);
	default:
		error("primary expected");
		// return 0; // actually works, but seems ugly
	}
}

[mbozzi]

What's with all the reporting? The OP post has also been erased.

Appears that @admin has restored the guy's account/post.

[deleted account xyzzy]

Quite a number of low-post-count new users had their opening posts reported/deleted recently. Now they are back, along with a major clean-up of block reporting on a lot of topics.

I hope the new users don't get turned off by the bad behavior of someone with the itchy report finger.

[mac10warrior]

Thanks for all the input.

Couple people pointed out that my solution of returning 0 does work. Like I tried to explain, I'm totally new to real programming, and am trying to really (like, really) understand everything i'm learning in the book.

Just silencing the warning by calling error() then returning 0 felt like a total hack.

For more context, the rest of the code is at - https://www.stroustrup.com/Programming/calculator08buggy.cpp
(it's an ongoing project that gets cleaned up throughout later chapters, fyi)

After fixing the bugs they put in, it compiles (with warnings) and works very well. Now I'm in day 3 of banging my head against the wall trying to parse a power function pow(double a, int b)...the damn comma is killing me.

thanks again. I've got a long way to go!

[Handy Andy]

Hello mac10warrior,

... trying to parse a power function pow(double a, int b)...the damn comma is killing me.

If above is what you are trying to do it is not the comma that is the problem it is the "double" and "int". This looks more like a function definition than a function call.

In a function call all you need is the variables name to use it:
returned Value = pow(2, 10); or returned Value = pow(a, b);. Using in a "cout":
std::cout << pow(a, b); would print the returned value.

Andy

[mac10warrior]

Thanks, Andy. No, it's part of this calculator code project. I wasn't being clear in that last comment - it was more of an aside. But in case you're interested, the drill was to add a power function to the calculator we have been working on.

All my programming experience before was in R. So doing this foundational stuff in the PPP book - thinking about errors and handling them well, dealing with user input, etc... is new to me.

Thanks again, everyone! Hoping to stay at it.