Skip to content
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

Add explicit keyword for generated constructor #386

Merged
merged 2 commits into from
Dec 11, 2023

Conversation

makotokato
Copy link
Contributor

https://bugzilla.mozilla.org/show_bug.cgi?id=1868324

I would like to add explicit keyword to generated C++ constructor.

@robertbastian
Copy link
Collaborator

Is this a breaking change for existing clients?

@makotokato
Copy link
Contributor Author

Our coding style needs explicit keyword for non copy/move constructor. So we have to modify it to use icu_capi since current FFI API doesn't accept our rule. explicit keyword can prevent unexpected conversion.

Also, see #349 for diplomat_runtime.hpp.

@robertbastian
Copy link
Collaborator

I understand the problem. My question is, can this change break compilation for anyone who is using ICU4X today?

@robertbastian
Copy link
Collaborator

I guess it can if someone is using implicit conversion today.

@Manishearth
Copy link
Contributor

It depends on whether you consider the raw pointer API to be actually public. I don't but we should talk about it.

@makotokato
Copy link
Contributor Author

I don't think adding explicit keyword breaks existed code. But feel free to reject this PR.

@Manishearth
Copy link
Contributor

It is for anyone using the constructor implicitly. I think that's not a big deal though, because that constructor is a major footgun.

@Manishearth Manishearth merged commit 5056b7b into rust-diplomat:main Dec 11, 2023
6 checks passed
@Manishearth
Copy link
Contributor

@sffc mind checking and fixing this in CPP2?

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

Successfully merging this pull request may close these issues.

3 participants