zend_portability: Remove EMPTY_SWITCH_DEFAULT_CASE()

[TimWolla]

This macro is exactly as long as explicitly spelling out its definition (excluding the useless break;), but hides control flow in its definition. This is giving tools like Coccinelle some troubles to understand the code and can also be confusing for the human reader, because it appears as if it is a statement that belongs to the current case rather than opening a new case.

[nielsdos]

Imo the tool should then be made smarter.
Damn I sound like a paper reviewer now :p

[alexdowad]

@TimWolla Thanks for working on this.

The PR message says that EMPTY_SWITCH_DEFAULT_CASE can be confusing to a human reader, but personally, I've never found it so. Is there evidence that a significant number of readers do find it confusing?

If not, then the sole remaining purpose of the PR is allowing Coccinelle to understand the code structure better. In that case, I suggest that some careful thinking and discussion would be in order, to make sure that this is a good and worthwhile change. Or maybe the discussion already occurred elsewhere?

[TimWolla]

Is there evidence that a significant number of readers do find it confusing?

“Significant” is probably hard to define, but the amount of macros in php-src is something that new contributors have often raised as something they struggle with.

While it is certainly easy to learn the macro definition (and I did so already), I find all macros that do not behave like simple constants or functions (with perhaps “iterator” macros that act like a loop) to introduce complexity that is often unnecessary.

In this specific case the macro is no shorter than its definition and it hides syntax, thus I don't see any value-add in this additional layer of indirection.

If not, then the sole remaining purpose of the PR is allowing Coccinelle to understand the code structure better.

Not just Coccinelle, but also other tools (e.g. formatters or coverage analysis tools).

[TimWolla]

And looking at the history of this macro: It predated the existence of ZEND_UNREACHABLE() and initially had different implementations depending on ZEND_DEBUG, where it could be argued that it provided a value-add:

92c4b06#diff-5a9720541c48f78a159b6806f9eb49b0a314032a6f2285c4dfa45b0de3180de0L113-R119

But since the introduction of ZEND_UNREACHABLE() in that commit, that value-add is gone and it seems unlikely that the macro would be newly introduced when it does not already exist, which I also find a good argument in favor of dropping it.

[alexdowad]

@TimWolla Well spoken.

[youkidearitai]

LGTM

[youkidearitai]

Just the mbstring side.

[DanielEScherzer]

okay for ext/reflection

[TimWolla]

@alexdowad Do I correctly understand your last comment as a “soft-approval” from your side?

[Girgias]

I'm not fundamentally against this change, but the argument that is confusing for human readers is rather untrue IMHO.

Who this macro confuses seems to only be tools, such as IDEs warning that the switch statement doesn't have a default case, or here the motivating factor being Coccinelle not being able to reason about the code properly.

And due to that, I mostly agree with @nielsdos in that the tools are the problem and should be smarter.

Thus the discussion really should be more about if we want support for those tools in their current state as they provide us more benefits than this code churn.

Also, if this would not be removed and not impact extensions this would also be somewhat different.

[TimWolla]

but the argument that is confusing for human readers is rather untrue IMHO.

To be clear: I was serious when making that point. To me it adds unnecessarily mental load seeing something that looks like a statement after a break;. Especially when it's indented as if it belongs to the previous case as done in mbstring:

php-src/ext/mbstring/mbstring.c

Line 5191 in ee43384

EMPTY_SWITCH_DEFAULT_CASE();

Also, if this would not be removed and not impact extensions this would also be somewhat different.

The alternative is available with PHP 8.0. I believe it is reasonable for extensions to keep up with changes in php-src instead of forcing php-src to carry around legacy for eternity.

Thus the discussion really should be more about if we want support for those tools in their current state as they provide us more benefits than this code churn.

I'm okay with tackling the discussion from that angle. What I want to avoid is writing RFCs for clean-up, hoping to ship this by informal agreement (i.e. a relevant number of approvals).