Update Diplomat to using cpp lib_name #6957
Conversation
|
|
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
Manishearth
force-pushed
the
diplomat-lib-name
branch
from
239c062 to
a62fb5d
Compare
Manishearth
force-pushed
the
diplomat-lib-name
branch
from
a62fb5d to
7946a07
Compare
Fixes #688, fixes #920. Projects are strongly encouraged to use this to avoid namespace pollution. With this change, C++ projects setting `lib_name` will use that as a toplevel namespace. Further namespaces produce sub-namespaces (e.g. ICU4X could have `lib_name = icu4x` and `namespace = properties` on some of its code for `icu4x::properties::blah`) Switching from namespaces to lib_name does not change the actual code too much EXCEPT `diplomat_runtime.h` will now be `libname::diplomat` and will have a lib-scoped include guard. This is overall better for everyone. This does not affect the nanobind backend, currently. Followup: #957 See this in action: - ICU4X: unicode-org/icu4x#6957 - temporal_rs: boa-dev/temporal#581
Manishearth
changed the title
|
|
| public: | ||
|
|
||
| /** | ||
| /** |
There was a problem hiding this comment.
Yeah but I think it's a good whitespace change?
There was a problem hiding this comment.
There was a problem hiding this comment.
Given the Diplomat architecture (this is behind three layers of nested templates) it becomes harder and harder to fix these things.
I think we should try and fix whitespace issues when we notice them if they're easy, but we should not care that much about them.
This change actually aligns the comment with itself: I noticed it when reviewing the Diplomat change and decided it was good. I made it better in rust-diplomat/diplomat#961, but I do not think I should be required to fix these things on Diplomat update PRs. It's often a huge waste of time mucking with Jinja whitespace controls to minimize the diff.
I agree with it when the diff starts pulling in unrelated files, but this diff was going to affect all files anyway.
There was a problem hiding this comment.
Updated. There is still a docs diff in this PR, but it is fixing the alignment as well and is intentional.
| std::fs::remove_dir_all(&include)?; | ||
| std::fs::create_dir(&include)?; | ||
| if lang == "cpp" { | ||
| include = include.join("icu4x"); |
There was a problem hiding this comment.
can diplomat create a library directory?
There was a problem hiding this comment.
I'd leave it up to the client. I considered it. I think it's a bit silly to unconditionally make a directory.
2 participants
Pulls in rust-diplomat/diplomat#956
We no longer mark all our FFI modules with namespaces. The code is largely unchanged, except that
diplomat_runtime.his now under theicu4xfolder, and alldiplomat::types areicu4x::diplomat::.This allows ICU4X to be mixed with other Diplomat-using libraries.