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

token_type_name(token_type t): not all control paths return a value #156

Closed
Drealmer opened this issue Dec 15, 2015 · 6 comments
Closed

Comments

@Drealmer
Copy link

Hello,

The function "token_type_name(token_type t)" contains a switch where every case contains a return, but Visual Studio 2015 complains about the function still having a control path that wouldn't return a value. Adding a default to the switch would fix it.

@nlohmann
Copy link
Owner

I saw this issue, but the error message makes no sense. Adding a default branch just to shut up the compiler is something I would call bad practice as it would just introduce unreachable code.

@Drealmer
Copy link
Author

Alright, thanks for your answer and sorry for bothering you.

@gregmarr
Copy link
Contributor

In the version that we're using, token_type_name has a default case, so if it doesn't have one now, it was recently removed
default:
return "<parse error>";

I see, it used to be default:, it was just changed to case token_type::parse_error:

The compiler warning about there being a control path that doesn't return is a good thing. It's only unreachable if you've already guaranteed that t doesn't contain a value outside of the specified enum values. It looks like you've done that through making sure that t can never contain something other than a valid enum value. Unfortunately, the compiler doesn't know that, and so can't tell that control doesn't reach the end of the function. There's been discussion on the C++ standards proposal group recently about how to tell the compiler that you know for sure that all paths through the switch return from the function, but so far there's nothing portable.

@nlohmann
Copy link
Owner

@gregmarr, sorry for the confusion - I overworked the switch statements lately as I thought I could get rid of all warnings. However, there seems to always be one compiler flag that yields a warning - even though the code is fine.

In the concrete setting, I did not want to replace the last case by default, because it would, from my point of view, make the code less readable.

@gregmarr
Copy link
Contributor

I'm fine with that, we may just need to add another warning to the list that we already suppress around the header: 4706 (assignment within conditional) and 4996 (function was declared deprecated). We have our own header that includes json.hpp, which is where we have our fix for the android strtold issue, so that's not a big issue.

@nlohmann
Copy link
Owner

Meanwhile, I saw that GCC 4.9 also warns about this.

@nlohmann nlohmann reopened this Dec 17, 2015
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