Drop redundant macro definitions from mbfl_defs.h #17114
Conversation
These are defined by the standard include headers we already use. These cause conflicts which cause compiler warnings on some toolchains (see phpGH-17112).
|
... This is a public header, seriously? Guess this is master-only then, which means that if people compile with Werror on lower branches and hit this it may break the build but we can't really break ABI either 🙄 |
|
Conflict is trivial and I'll fix it in the merge, I can't be bothered |
I don't quite understand why. These are guarded with Still, of course it makes sense to remove that.
See #16070. We've been too lazy in the past, and often exported all header files. In this case, it might be okay to drop the conditional macro definitions for stable branches (more a theoretic API break than an ABI break, anyway). |
Because it's the opposite right? First this thing gets included and then the standard header. At least that's my understanding. |
|
Oh, indeed! php-src/ext/mbstring/libmbfl/mbfl/mbfl_encoding.h Lines 34 to 36 in a7785e8 Moving the zend.h include to the top might solve the issue, too (besides that I consider it somewhat doubtful that we're even including zend.h in this "module"). |
|
That would work indeed, but it's better to get rid of the redundancy altogether 🙂 |
|
Thanks very much for noticing this issue! I'm sure there is a lot of other "cruft" in mbstring which could be cleaned out. |
|
@nielsdos, I fully agree that we should drop that nonsentical fallback definitions, but perhaps it's "safer" to only change the include order for stable versions. Or maybe we should do both right away. |
|
We can change the include order for stable |
These are defined by the standard include headers we already use. These cause conflicts which cause compiler warnings on some toolchains (see GH-17112).
This fixes the mbstring part of the issue.