- removed comment
incorrect use of CCTK_BUILTIN_UNREACHABLE in Carpet
Issue #1571
closed
Carpet in commit fc35c561a049d905763de37d60690fc15473d65d "CarpetLib: Some harmless code cleanup" (sic) replaced some assert(0) by CCTK_BUILTIN_UNREACHABLE().
One of those (in the switch in transfer_p_r in line 730 of data.cc) was just found by Zach Etienne to cause segfaults if one does not properly add a new interpolation operator.
The difference is that CCTK_BUILTIN_UNREACHABLE() tells the compiler that a given line of code will never be reached thus the compiler can optimize it away. assert(0) on the other hand makes no such statement, it just aborts if it is reached. This error is compiler dependent since CCTK_BUILTIN_UNREACHABLE() expands to CCTK_Abort(0,1) unless the compiler supports __builtin_unreachable.
Keyword:
Comments (3)
-
-
- changed status to resolved
- removed comment
I replaced the CCTK_BUILTIN_UNREACHABLE() by assert(0) again.
-
reporter
- edited description
- changed status to closed
- Log in to comment
- Assignee
- Type
- Priority
- Status
- closed
- Component
- Carpet
- Milestone
- –
- Version
- development version
- Votes
- 0
- Watchers
- 0
One should use builtin_unreachable only in cases where code is indeed manifestly unreachable (to avoid compiler warnings), or in performance critical code (where one wouldn't want and assert either).
You are right, this case is neither. There may be operators that prolongate in some directions, but not in others. Of course, this doesn't make sense in Carpet, but this case should be caught with an assert.