POC: Rust Wrapper for 1DS C++ SDK #1225
Conversation
|
@microsoft-github-policy-service agree company="Microsoft" |
| } | ||
|
|
||
| fn log(&self, record: &Record) { | ||
| if unsafe { CONSOLE_ENABLED } && self.enabled(record.metadata()) { |
There was a problem hiding this comment.
These fields should probably be changed to use std::sync::atomc::AtomicBool, which will allow this code to be safe.
Using static mut is unsafe because it allows for race conditions, which are undefined behavior.
| pub fn to_leaky_c_str(value: &str) -> *const i8 { | ||
| let c_str = Box::new(CString::new(value.clone()).unwrap()); | ||
| let ptr = c_str.as_ptr() as *const i8; | ||
| mem::forget(c_str); |
There was a problem hiding this comment.
Can this be Box::into_raw(c_str) as *const i8 instead?
| } | ||
|
|
||
| /// The global `Tracer` provider singleton. | ||
| static GLOBAL_LOG_MANAGER: Lazy<RwLock<LogManager>> = Lazy::new(|| RwLock::new(LogManager::new())); |
There was a problem hiding this comment.
Is there a reason this uses Lazy instead of just typing this field as RwLock<LogManager>?
| * This will leak memory | ||
| */ | ||
| pub fn to_leaky_c_str(value: &str) -> *const i8 { | ||
| let c_str = Box::new(CString::new(value.clone()).unwrap()); |
There was a problem hiding this comment.
I suspect the value.clone() call doesn't do anything. It could probably be simplified to just value.
| } | ||
|
|
||
| fn to_str(&self) -> Result<&str, std::str::Utf8Error> { | ||
| unsafe { CStr::from_ptr(self.as_ptr()).to_str() } |
There was a problem hiding this comment.
Is there a reason this is not just core::str::from_utf8(&self.bytes)? What merits using unsafe code here?
| result | ||
| } | ||
|
|
||
| pub fn evt_upload(handle: &evt_handle_t) -> evt_status_t { |
There was a problem hiding this comment.
Because you are directly exposing evt_handle_t outside your crate, and this type has unsafe pointers in it, these functions are actually unsafe to call. They allow a safe caller to violate type safety.
You can fix this in one of several ways:
- Don't expose these functions outside this crate.
- Don't expose
evt_context_t, and instead define anEvtContextwrapper type which containsevt_context_tbut does not expose it outside of the crate. - Mark these functions
unsafe.
| [dependencies] | ||
| chrono = { version = "0.4.30" } | ||
| log = { version = "0.4.20" } | ||
| once_cell = "1.12.0" |
There was a problem hiding this comment.
Is there a reason to use once_cell, now that Once and OnceLock are part of the Rust standard library?
| pub mod appender; | ||
| pub mod types; | ||
|
|
||
| #[derive(Clone)] |
There was a problem hiding this comment.
Is log_handle a pointer or a handle that was received from the underlying C API? If so, then I don't think this Clone call is correct or safe.
| Int(i64), | ||
| } | ||
|
|
||
| pub type TelemetryPropertyBag = HashMap<&'static str, TelemetryProperty>; |
There was a problem hiding this comment.
It's a matter of taste, but I generally prefer to simply see HashMap<&str, TelemetryProperty> instead of a type alias.
| TelemetryItem { | ||
| name: StringProperty::new(name), | ||
| data: TelemetryPropertyBag::new(), | ||
| time: Option::None, |
|
@CDFriend This looks to be a good addition. when you have time, can you please make it ready for review :) |
* Making quality of life improvements to the types * Adding testing to types
msft-tsharp
force-pushed
the
cfriend/rust-wrapper-contd
branch
from
a95e623
to
5c3a0ef
Compare
What is this?
Adds a Rust "wrapper" to the 1DS C++ SDK, which will allow the SDK to be imported into Rust projects as a crate. The crate interacts with the C API of the SDK (instead of C++) as this will ensure a stable binary interface between the Rust layer and C++ layer of the project.
Project Structure
Crates:
Limitations
i64andstringtelemetry fields are supported.Build Instructions
Win32-Libconfiguration for the x64 arch.