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

Namespacing for C++ #427

Closed
2 of 3 tasks
Manishearth opened this issue Feb 13, 2024 · 6 comments · Fixed by #512
Closed
2 of 3 tasks

Namespacing for C++ #427

Manishearth opened this issue Feb 13, 2024 · 6 comments · Fixed by #512
Assignees
Labels
B-cpp C++ backend
Milestone

Comments

@Manishearth
Copy link
Contributor

Manishearth commented Feb 13, 2024

Sub-issue of #103

Steps:

@Manishearth
Copy link
Contributor Author

Manishearth commented Jun 4, 2024

I don't think we ever recorded the results of this discussion properly (or if we did, it's in an issue we can't find), but the consensus between @sffc , @robertbastian , and I, if I recall correctly, was:

  • We no longer have #ifdef __cplusplus anywhere. People wishing to use the C headers from C++ must wrap the include in extern "C", which is standard practice. (Get rid of __cplusplus ifdefs #490)
  • The cpp-c2 headers become semi-private/internal headers that cannot be used from C anymore. You can still type cast and use the C API when needed.
  • cpp-c2 code is now namespaced just like the cpp code, though it lives under an additional capi namespace (e.g. icu4x::properties::foo vs icu4x::properties::capi::foo

This means that C++ clients of ICU4X get no global namespace pollution, and C clients don't have to deal with C++-specific stuff. Clients using both can use both, if they wish to share types between both it is a little bit of work but that kind of type sharing is only marginally supported anyway.

@sffc
Copy link
Contributor

sffc commented Jun 4, 2024

Some discussion:

  • Does this work with the ODR?
  • Would be good to inline the C decls into the C++ header files so we have just 2: .d.hpp and .hpp. It creates some implementation complications but the outcome is nicer.
  • Is there something weird about how this interacts with results? It sounds like we need to refactor how we do results.

Conclusion: If possible, try to inline C decls and result types into the header files so there are no more .h files. Otherwise, follow plan to stuff .h files under capi/.

@Manishearth @robertbastian @sffc @younies

@Manishearth
Copy link
Contributor Author

#512 finishes all the stuff I consider mandatory here.

@sffc how important is it for namespaced headers to also be organized in folders named after the namespace? The main reason to do so is to allow name conflicts, but we could also handle that some other way (allow people to override the header file name with an attribute).

It's a change we can only make pre-2.0, but the more I think about it the less necessary it seems to me. Most c++ libraries seem to organize their headers flat (mostly)

@sffc
Copy link
Contributor

sffc commented Jun 27, 2024

Hmm, flat is probably fine; fewer decisions to make.

@Manishearth
Copy link
Contributor Author

In that case I think #512 fixes this!

@Manishearth
Copy link
Contributor Author

There's quality of life stuff we can do from here on, like the ability to pass in a default namespace from the CLI, but that's not mandatory for stability.

@Manishearth Manishearth added this to the ICU4X 2.0 milestone Jun 28, 2024
@Manishearth Manishearth added the B-cpp C++ backend label Jun 28, 2024
This was referenced Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-cpp C++ backend
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants