Skip to content

ext/intl: migrate C code to C++ step 3. #19411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devnexen
Copy link
Member

@devnexen devnexen commented Aug 8, 2025

No description provided.

@devnexen devnexen marked this pull request as ready for review August 8, 2025 08:20
@bukka
Copy link
Member

bukka commented Aug 9, 2025

What's the actual reason for all of this?

@devnexen
Copy link
Member Author

devnexen commented Aug 9, 2025

couple of reasons :
1/ icu C++ api is the main api, C wrappers versions of new features come later (when they do).
2/ mixing C++ with cumbersome procedural C makes the code more complicated (esp. memory management,e.g. zend mm and C++ objects, lesser chances of leaks).
3/ no more need to export C++ symbols as C.
4/ C++ has stronger typing, can be annoying again to mix with C "looseness".

@bukka
Copy link
Member

bukka commented Aug 9, 2025

It seems like it cought up from the https://unicode-org.github.io/icu-docs/apidoc/released/icu4c/ . It's just few things missing from that list.

But if you really think it's worth the time and can bring some significant features / reduce number of bugs then I will leave it up to you.

@bukka
Copy link
Member

bukka commented Aug 9, 2025

One thing to note is that it would be good to still keep it understandable for C developers and not try to introduce some C++ features just for the sake of it. I don't have personally have problem to understand C++ which I guess was the reason why the extension was written in really C way.

@bukka
Copy link
Member

bukka commented Aug 9, 2025

That comment was more about type of changes in #19429 rather than this though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants