-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Minor: fix msvc warning "not all control paths return a value" #5650
Changes from 2 commits
79b6525
9d2d226
7bcdf4a
b3e07d4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,7 +57,12 @@ | |
# define NORETURN [[noreturn]] | ||
#elif defined(__GNUC__) | ||
# define NORETURN __attribute__((noreturn)) | ||
#else | ||
#elif defined __has_cpp_attribute | ||
# if __has_cpp_attribute (noreturn) | ||
# define NORETURN [[noreturn]] | ||
# endif | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer if we keep the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, but how do we handle the case of a compiler which supports The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This comment is indeed in contradiction with my other comment. I realized it myself shortly after I wrote it. But we could move the check for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You mean like
If that's what you meant, I fail to see a benefit but it would work fine as well. If you feel strongly about it, I'll modify it like that. If you meant something else, could you please clarify? I'm not sure I got your idea. Just setting a different '#define' for whether There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ah right. I forgot about this annoying quirk. As I mentioned my experiences with post-c+11 code bases are few.
I don't. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the kind of solution I was looking for was something we already use in #ifndef __has_builtin
#define __has_builtin(x) 0 // Compatibility with non-clang compilers.
#endif There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm that's not standard compliant code, is it? I'd be careful defining symbols starting with "__". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it has nothing to do with the standard but There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I admit that the following is only language lawyer nitpicking and I think we'll be fine in practice the way cppcheck is now using it. But if you feel like reading an argument partly against it anyway, here it goes: Yes, I think you're not getting the reserved identifier warning because you're testing with a clang version that supports A program which Standard library implementations also do work with those identifiers but I did not find a case that you'd be breaking by defining them like you're doing. I actually came across a case just like ours on isocpp.org. Still to my understanding it's not in line with the letter of the standard. |
||
#if !defined(NORETURN) | ||
# define NORETURN | ||
#endif | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be the first to be checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and have moved it up.