Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dashboard: bad warning for return without value. #2073

Closed
leamas opened this issue Oct 23, 2020 · 14 comments
Closed

dashboard: bad warning for return without value. #2073

leamas opened this issue Oct 23, 2020 · 14 comments

Comments

@leamas
Copy link
Collaborator

leamas commented Oct 23, 2020

As heading says:

/home/mk/OpenCPN/OpenCPN/plugins/dashboard_pi/src/dashboard_pi.cpp: In function ‘double GetJsonDouble(wxJSONValue&)’:
/home/mk/OpenCPN/OpenCPN/plugins/dashboard_pi/src/dashboard_pi.cpp:540:1: warning: control reaches end of non-void function [-Wreturn-type]

Nasty one, this. Will be patched using attached patch in Debian package

0006-dashboard_pi-Handle-return-without-value-warning.patch.gz

@Hakansv
Copy link
Contributor

Hakansv commented Oct 23, 2020

Will probably be solved by: #2020

@leamas
Copy link
Collaborator Author

leamas commented Oct 23, 2020

Indeed, thanks.

@Hakansv
Copy link
Contributor

Hakansv commented Oct 23, 2020

You know Win is quit forgiving but these Unix based system are sometimes idiosyncratic. (Nice word I found or?)

@leamas
Copy link
Collaborator Author

leamas commented Oct 23, 2020

Don't know about the word. But the more bugs the compiler catches, the better. And this is definitely a bug. So we are lucky building on multiple platforms so we can get these kind of warnings from many compilers.

Does this really pass mscv without warnings? If so, it's kind of interesting to think about the code it generated ;)

@Hakansv
Copy link
Contributor

Hakansv commented Oct 23, 2020

Yes it compiles fine using VS2017 but I got the warning when tested the patch on my RPi

@leamas
Copy link
Collaborator Author

leamas commented Oct 23, 2020

It's interesting, but not interesting enough to review the emitted code. That said, I couldn't really fathom would that would be..

@Hakansv
Copy link
Contributor

Hakansv commented Oct 23, 2020

This would be a no problem I suppose. Most code are more or less tested on all system. Well, I've no Mac so can't test on that but if in doubts I use to ask for it.

@leamas
Copy link
Collaborator Author

leamas commented Oct 23, 2020

Tested or not, I cannot imagine what code a compiler would generate for

double foo(int arg) {
  if (arg < 10) return 1;
  if (arg > 100 return 2
}

We can of course make sure that we never call this function with an argument in the 10..100 range. But, what's in the the relevant register (or on the stack) at the end of that function is undefined, and so is the function value. This is a bug on any compiler, and if mscv does not emit a warning for it it's IMHO a bug in msvc.

OTOH, IIRC some warnings are disabled in the builds; perhaps this is the reason

@Hakansv
Copy link
Contributor

Hakansv commented Oct 23, 2020

VS2017 says: warning C4715: 'foo': not all control paths return a value
But an intelligent compiler would make a arg = 50 replying NAN? :)

@leamas
Copy link
Collaborator Author

leamas commented Oct 23, 2020

VS2017 says: warning C4715: 'foo': not all control paths return a value

Yes, this is the sane thing to do

But an intelligent compiler would make a arg = 50 replying NAN? :)

No way, only a really dumb compiler does that. There are no provisions in the C++ standard for it, and a compiler should implement the standard. Return NaN in this scenario could cause all sort of surprises for the calling code, surprises you cannot understand when reading the code.

@Hakansv
Copy link
Contributor

Hakansv commented Oct 23, 2020

OK sound fair. But now we're over my head. Nice chat. End?

@leamas
Copy link
Collaborator Author

leamas commented Oct 23, 2020

Right. Have a nice weekend :)

@bdbcat
Copy link
Member

bdbcat commented Oct 23, 2020

"idiosyncratic"
Nice word. appropriate here.

@leamas
Copy link
Collaborator Author

leamas commented Jan 29, 2021

fixed in #2020. closing

@leamas leamas closed this as completed Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants