Address more Clang warnings #17506
Address more Clang warnings #17506
Conversation
We prefer clean solutions (such as declaring the proper type in the first place, or introducing a portable format specifier) where easily possible, but resort to casts otherwise.
cmb69
requested review from
bukka,
kamil-tekiela,
SakiTakamachi,
nielsdos,
devnexen and
dstogov
as code owners
| @@ -353,7 +353,7 @@ static zend_function *com_method_get(zend_object **object_ptr, zend_string *name | |||
| ITypeComp_Release(bindptr.lptcomp); | |||
| break; | |||
|
|
|||
| case DESCKIND_NONE: | |||
| default: | |||
There was a problem hiding this comment.
Perhaps EMPTY_SWITCH_DEFAULT_CASE() instead?
There was a problem hiding this comment.
That macro is a misnomer and dangerous, it asserts the path is unreachable
There was a problem hiding this comment.
it asserts the path is unreachable
Yes and this seems to be the desired behavior here. The documentation indicates that only the 4 DESCKIND_* cases that were previously handled may be returned. I'd rather crash if a new case appears instead of silently doing the wrong thing with it.
There was a problem hiding this comment.
The thing is, you won't necessarily crash. Anything can happen because taking an unreachable path is UB. This can either result in taking the wrong case or doing something completely absurd.
There was a problem hiding this comment.
Remember that assertions are turned into assumes in release builds.
There was a problem hiding this comment.
I'd be fine with an assert(), but __assume() is dangerous, it's comparable to UB, if applied at the wrong place, and I wouldn't fully trust that documentation. Maybe in this case it is better to explicitly list the alternatives, and add a ZEND_ASSERT(0) there.
There was a problem hiding this comment.
ZEND_ASSERT(0) essentially would do the same thing as marking it unreachable. I'd be okay with assert(0) but I predict someone's gonna turn that into ZEND_ASSERT on refactor in the future. ZEND_ASSERT is turned into an assume, and with 0 as an argument it's an assume on something that's false, the compiler can do anything. Can we please not do dangerous programming?
There was a problem hiding this comment.
D'oh! Of course, you're right.
Maybe we should actually change ZEND_ASSERT() to not be ZEND_ASSUME() in non-debug builds, and use ZEND_ASSUME() directly, where we are absolutely sure that the code is unreachable (in which case we can likely drop such code, and maybe address compiler warnings by other means). Or something like that.
There was a problem hiding this comment.
I did not know the difference between assume and assert, for me the assertions are there to ensure in debug builds that the preconditions, or the current state is what you expect it to be.
There was a problem hiding this comment.
Transforming assertions into assumes was done on purpose during the phpng development: e1ab2cf
Changing that would have a performance impact as people specifically use it in that way (debug time assertions, production time optimizations)
|
Yeah, let's merge; there are still so many warnings to address. |
We prefer clean solutions (such as declaring the proper type in the first place, or introducing a portable format specifier) where easily possible, but resort to casts otherwise.