Minor cleanups to firebase_assert.h #1051
Conversation
wilhuff
force-pushed
the
wilhuff/unreachable
branch
from
1b2dbf6 to
e05f25d
Compare
| @@ -76,10 +76,6 @@ | |||
| } \ | |||
| } while (0) | |||
|
|
|||
| // Assert with custom message that is not compiled out in release builds. | |||
There was a problem hiding this comment.
Was removing this on purpose? (Seems otherwise unrelated to this PR.)
There was a problem hiding this comment.
It's a duplicate definition that I cleaned up while in here.
| @@ -106,18 +102,26 @@ | |||
| // Indicates an area of the code that cannot be reached (except possibly due to | |||
| // undefined behaviour or other similar badness). The only reasonable thing to | |||
| // do in these cases is to immediately abort. | |||
| #if defined(__clang__) || defined(__GNUC__) | |||
| // Clang, GCC, (and Intel) compilers have a builtin | |||
| #define FIREBASE_UNREACHABLE() __builtin_unreachable() | |||
There was a problem hiding this comment.
This (and __assume(0) below) fixes the compilation error (just like abort()) but unlike abort(), this will lead to undefined behaviour if the code is ever actually reached.
Assuming the reason FIREBASE_UNREACHABLE is called is due to an exhaustive switch statement, this would imply we're already in (mostly) undefined behaviour land, so this (probably) is no worse. But it means instead of crashing, the program will ... probably still crash immediately. But I suppose it could continue on, possibly returning arbitrary nonsense back to the caller.
Is this ok?
There was a problem hiding this comment.
My attempts to force this to happen essentially showed that the program does crash immediately.
The win of using __builtin_unreachable() is:
- the compiler can omit checks for whether or not the switch value is one that's handled
- the compiler doesn't emit code to call
abort() - code coverage doesn't "see" this as an actual case we never test
So from the strict point of view of just silencing the warning about the return value this is better, but yeah it's possible to end up here through programmer error and the result isn't as obviously useful as an abort() (it's a SIGSEGV on the mac with clang, but the stack is smashed and unreadable).
wilhuff
changed the title
Remove a duplicate macro definition and clean up an extra const clang-tidy complains about.
Remove a duplicate macro definition and clean up an extra
constclang-tidy complains about.