Rust bindings (eckit-sys crate)#289
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #289 +/- ##
===========================================
- Coverage 66.41% 66.41% -0.01%
===========================================
Files 1128 1128
Lines 57925 57926 +1
Branches 4413 4413
===========================================
Hits 38470 38470
- Misses 19455 19456 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
simondsmart
left a comment
simondsmart
left a comment
There was a problem hiding this comment.
Minor comments/changes/questions.
| eckit-sys = { path = "crates/eckit-sys" } | ||
|
|
||
| # Build tools | ||
| bindman = { git = "ssh://git@github.com/ecmwf/bindman.git" } |
There was a problem hiding this comment.
Do we need/want to pin the version of these?
| This crate provides raw FFI bindings using [cxx](https://cxx.rs/). For a safe, | ||
| ergonomic API, use the higher-level `eckit` crate (forthcoming). | ||
|
|
||
| ## Features |
There was a problem hiding this comment.
Can you check this please, as there is a lot of functionality listed below which is not made available in the rust bindings.
There was a problem hiding this comment.
They're not Rust API features, they're a 1:1 mapping to eckit's CMake ENABLE_* flags that gate which optional C++ components get built into the vendored library.
Fixed in 0ed608e
| // ==================== DataHandle ==================== | ||
|
|
||
| /// Wraps `eckit::DataHandle*` for Rust FFI. Takes ownership. | ||
| class DataHandleWrapper { |
There was a problem hiding this comment.
Seeing all of these classes in this one header, do we want to split this into multiple topic related header files, to make this more digestable?
| }; | ||
|
|
||
| // Factory | ||
| std::unique_ptr<ReaderWrapper> new_reader(DataHandleWrapper& handle); |
There was a problem hiding this comment.
Should these be free functions, or would they not be cleaner as static functions in the DataHandleWrapper class?
| class StreamWrapper { | ||
| protected: | ||
|
|
||
| eckit::Stream* stream_ = nullptr; |
There was a problem hiding this comment.
Why is this a raw pointer here? If it is owning, then it should be unique_ptr. If it is not, then it should hold the stream by reference.
| RustMain::RustMain(int argc, char** argv) : Main(argc, argv) {} | ||
|
|
||
| eckit::LogTarget* RustMain::createInfoLogTarget() const { | ||
| return new RustLogTarget(LogLevel::Info, "eckit"); |
There was a problem hiding this comment.
I'm not sure that "eckit" is the right name for these targets. They are application level targets, and should probably take their name from the calling code?
| if (!eckit::Main::ready()) { | ||
| static const char* argv[] = {"eckit-rs", nullptr}; | ||
| static auto* main = new RustMain(1, const_cast<char**>(argv)); | ||
| (void)main; |
There was a problem hiding this comment.
What is this (void) main for?
3 participants
Description
Contributor Declaration
By opening this pull request, I affirm the following:
🌦️ >> Documentation << 🌦️
https://sites.ecmwf.int/docs/dev-section/eckit/pull-requests/PR-289