C++ ingest string views instead of const char* (almost) everywhere

.typos.toml

@@ -117,6 +117,12 @@ valour = "valor"
 vaporise = "vaporize"
 vigour = "vigor"
 
+# null-terminated is the name of the wikipedia article!
+# https://en.wikipedia.org/wiki/Null-terminated_string
+nullterminated = "null-terminated"
+zeroterminated = "null-terminated"
+zero-terminated = "null-terminated"
+
 [default]
 # Work around for typos inside of 8-character hashes. These show up inside of ipynb.
 # e.g. "f4e1caf9" -> `caf` should be `calf`

CODE_STYLE.md

@@ -189,6 +189,11 @@ class Rect {
 }
 ```
 
+### String handling
+Whenever possible we use `std::string_view` to pass strings.
+
+To accommodate for this and other languages, strings on the C interface are almost never expected to be null-terminated and are always passed along with a byte length using `rr_string`.
+
 ### Misc
 We don't add `inline` before class/struct member functions if they are inlined in the class/struct definition.
 

crates/rerun_c/src/lib.rs

@@ -22,16 +22,35 @@ use re_sdk::{
 // ----------------------------------------------------------------------------
 // Types:
 
+/// This is called `rr_string` in the C API.
+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+pub struct CStringView {
+    pub string: *const c_char,
+    pub length: u32,
+}
+
+impl CStringView {
+    #[allow(clippy::result_large_err)]
+    pub fn as_str<'a>(&'a self, argument_name: &'a str) -> Result<&'a str, CError> {
+        ptr::try_char_ptr_as_str(self.string, self.length, argument_name)
+    }
+
+    pub fn is_null(&self) -> bool {
+        self.string.is_null()
+    }
+}
+
 type CRecordingStream = u32;
 
 /// C version of [`re_sdk::SpawnOptions`].
 #[derive(Debug, Clone)]
 #[repr(C)]
 pub struct CSpawnOptions {
     pub port: u16,
-    pub memory_limit: *const c_char,
-    pub executable_name: *const c_char,
-    pub executable_path: *const c_char,
+    pub memory_limit: CStringView,
+    pub executable_name: CStringView,
+    pub executable_path: CStringView,
 }
 
 impl CSpawnOptions {
@@ -44,18 +63,16 @@ impl CSpawnOptions {
         }
 
         if !self.memory_limit.is_null() {
-            spawn_opts.memory_limit =
-                ptr::try_char_ptr_as_str(self.memory_limit, "memory_limit")?.to_owned();
+            spawn_opts.memory_limit = self.memory_limit.as_str("memory_limit")?.to_owned();
         }
 
         if !self.executable_name.is_null() {
-            spawn_opts.executable_name =
-                ptr::try_char_ptr_as_str(self.executable_name, "executable_name")?.to_owned();
+            spawn_opts.executable_name = self.executable_name.as_str("executable_name")?.to_owned();
         }
 
         if !self.executable_path.is_null() {
             spawn_opts.executable_path =
-                Some(ptr::try_char_ptr_as_str(self.executable_path, "executable_path")?.to_owned());
+                Some(self.executable_path.as_str("executable_path")?.to_owned());
         }
 
         Ok(spawn_opts)
@@ -86,14 +103,14 @@ impl From<CStoreKind> for StoreKind {
 #[derive(Debug)]
 pub struct CStoreInfo {
     /// The user-chosen name of the application doing the logging.
-    pub application_id: *const c_char,
+    pub application_id: CStringView,
 
     pub store_kind: CStoreKind,
 }
 
 #[repr(C)]
 pub struct CDataCell {
-    pub component_name: *const c_char,
+    pub component_name: CStringView,
 
     /// Length of [`Self::bytes`].
     pub num_bytes: u64,
@@ -104,7 +121,7 @@ pub struct CDataCell {
 
 #[repr(C)]
 pub struct CDataRow {
-    pub entity_path: *const c_char,
+    pub entity_path: CStringView,
     pub num_instances: u32,
     pub num_data_cells: u32,
     pub data_cells: *const CDataCell,
@@ -239,7 +256,7 @@ fn rr_recording_stream_new_impl(store_info: *const CStoreInfo) -> Result<CRecord
         store_kind,
     } = *store_info;
 
-    let application_id = ptr::try_char_ptr_as_str(application_id, "store_info.application_id")?;
+    let application_id = application_id.as_str("store_info.application_id")?;
 
     let mut rec_builder = RecordingStreamBuilder::new(application_id)
         //.is_official_example(is_official_example) // TODO(andreas): Is there a meaningful way to expose this?
@@ -330,12 +347,12 @@ pub extern "C" fn rr_recording_stream_flush_blocking(id: CRecordingStream) {
 #[allow(clippy::result_large_err)]
 fn rr_recording_stream_connect_impl(
     stream: CRecordingStream,
-    tcp_addr: *const c_char,
+    tcp_addr: CStringView,
     flush_timeout_sec: f32,
 ) -> Result<(), CError> {
     let stream = recording_stream(stream)?;
 
-    let tcp_addr = ptr::try_char_ptr_as_str(tcp_addr, "tcp_addr")?;
+    let tcp_addr = tcp_addr.as_str("tcp_addr")?;
     let tcp_addr = tcp_addr.parse().map_err(|err| {
         CError::new(
             CErrorCode::InvalidSocketAddress,
@@ -357,7 +374,7 @@ fn rr_recording_stream_connect_impl(
 #[no_mangle]
 pub extern "C" fn rr_recording_stream_connect(
     id: CRecordingStream,
-    tcp_addr: *const c_char,
+    tcp_addr: CStringView,
     flush_timeout_sec: f32,
     error: *mut CError,
 ) {
@@ -409,9 +426,9 @@ pub extern "C" fn rr_recording_stream_spawn(
 #[allow(clippy::result_large_err)]
 fn rr_recording_stream_save_impl(
     stream: CRecordingStream,
-    path: *const c_char,
+    path: CStringView,
 ) -> Result<(), CError> {
-    let path = ptr::try_char_ptr_as_str(path, "path")?;
+    let path = path.as_str("path")?;
     recording_stream(stream)?.save(path).map_err(|err| {
         CError::new(
             CErrorCode::RecordingStreamSaveFailure,
@@ -424,7 +441,7 @@ fn rr_recording_stream_save_impl(
 #[no_mangle]
 pub extern "C" fn rr_recording_stream_save(
     id: CRecordingStream,
-    path: *const c_char,
+    path: CStringView,
     error: *mut CError,
 ) {
     if let Err(err) = rr_recording_stream_save_impl(id, path) {
@@ -435,10 +452,10 @@ pub extern "C" fn rr_recording_stream_save(
 #[allow(clippy::result_large_err)]
 fn rr_recording_stream_set_time_sequence_impl(
     stream: CRecordingStream,
-    timeline_name: *const c_char,
+    timeline_name: CStringView,
     sequence: i64,
 ) -> Result<(), CError> {
-    let timeline = ptr::try_char_ptr_as_str(timeline_name, "timeline_name")?;
+    let timeline = timeline_name.as_str("timeline_name")?;
     recording_stream(stream)?.set_time_sequence(timeline, Some(sequence));
     Ok(())
 }
@@ -447,7 +464,7 @@ fn rr_recording_stream_set_time_sequence_impl(
 #[no_mangle]
 pub extern "C" fn rr_recording_stream_set_time_sequence(
     stream: CRecordingStream,
-    timeline_name: *const c_char,
+    timeline_name: CStringView,
     sequence: i64,
     error: *mut CError,
 ) {
@@ -459,10 +476,10 @@ pub extern "C" fn rr_recording_stream_set_time_sequence(
 #[allow(clippy::result_large_err)]
 fn rr_recording_stream_set_time_seconds_impl(
     stream: CRecordingStream,
-    timeline_name: *const c_char,
+    timeline_name: CStringView,
     seconds: f64,
 ) -> Result<(), CError> {
-    let timeline = ptr::try_char_ptr_as_str(timeline_name, "timeline_name")?;
+    let timeline = timeline_name.as_str("timeline_name")?;
     recording_stream(stream)?.set_time_seconds(timeline, Some(seconds));
     Ok(())
 }
@@ -471,7 +488,7 @@ fn rr_recording_stream_set_time_seconds_impl(
 #[no_mangle]
 pub extern "C" fn rr_recording_stream_set_time_seconds(
     stream: CRecordingStream,
-    timeline_name: *const c_char,
+    timeline_name: CStringView,
     seconds: f64,
     error: *mut CError,
 ) {
@@ -483,10 +500,10 @@ pub extern "C" fn rr_recording_stream_set_time_seconds(
 #[allow(clippy::result_large_err)]
 fn rr_recording_stream_set_time_nanos_impl(
     stream: CRecordingStream,
-    timeline_name: *const c_char,
+    timeline_name: CStringView,
     nanos: i64,
 ) -> Result<(), CError> {
-    let timeline = ptr::try_char_ptr_as_str(timeline_name, "timeline_name")?;
+    let timeline = timeline_name.as_str("timeline_name")?;
     recording_stream(stream)?.set_time_nanos(timeline, Some(nanos));
     Ok(())
 }
@@ -495,7 +512,7 @@ fn rr_recording_stream_set_time_nanos_impl(
 #[no_mangle]
 pub extern "C" fn rr_recording_stream_set_time_nanos(
     stream: CRecordingStream,
-    timeline_name: *const c_char,
+    timeline_name: CStringView,
     nanos: i64,
     error: *mut CError,
 ) {
@@ -508,9 +525,9 @@ pub extern "C" fn rr_recording_stream_set_time_nanos(
 #[allow(clippy::result_large_err)]
 fn rr_recording_stream_disable_timeline_impl(
     stream: CRecordingStream,
-    timeline_name: *const c_char,
+    timeline_name: CStringView,
 ) -> Result<(), CError> {
-    let timeline = ptr::try_char_ptr_as_str(timeline_name, "timeline_name")?;
+    let timeline = timeline_name.as_str("timeline_name")?;
     recording_stream(stream)?.set_time_sequence(timeline, None);
     Ok(())
 }
@@ -519,7 +536,7 @@ fn rr_recording_stream_disable_timeline_impl(
 #[no_mangle]
 pub extern "C" fn rr_recording_stream_disable_timeline(
     stream: CRecordingStream,
-    timeline_name: *const c_char,
+    timeline_name: CStringView,
     error: *mut CError,
 ) {
     if let Err(err) = rr_recording_stream_disable_timeline_impl(stream, timeline_name) {
@@ -553,7 +570,7 @@ fn rr_log_impl(
         data_cells,
     } = *data_row;
 
-    let entity_path = ptr::try_char_ptr_as_str(entity_path, "entity_path")?;
+    let entity_path = entity_path.as_str("entity_path")?;
     let entity_path = EntityPath::parse_forgiving(entity_path);
 
     re_log::debug!(
@@ -570,8 +587,7 @@ fn rr_log_impl(
             bytes,
         } = *data_cell;
 
-        let component_name =
-            ptr::try_char_ptr_as_str(component_name, "data_cells[i].component_name")?;
+        let component_name = component_name.as_str("data_cells[i].component_name")?;
         let component_name = ComponentName::from(component_name);
 
         let bytes = unsafe { std::slice::from_raw_parts(bytes, num_bytes as usize) };

crates/rerun_c/src/ptr.rs

@@ -1,6 +1,6 @@
-use std::ffi::{c_char, CStr};
+use std::ffi::c_char;
 
-use crate::CError;
+use crate::{CError, CErrorCode};
 
 #[allow(unsafe_code)]
 #[allow(clippy::result_large_err)]
@@ -16,10 +16,28 @@ pub fn try_ptr_as_ref<T>(ptr: *const T, argument_name: &str) -> Result<&T, CErro
 /// Tries to convert a [`c_char`] pointer to a string, raises an error if the pointer is null or it can't be converted to a string.
 #[allow(unsafe_code)]
 #[allow(clippy::result_large_err)]
-pub fn try_char_ptr_as_str(ptr: *const c_char, argument_name: &str) -> Result<&str, CError> {
+pub fn try_char_ptr_as_str(
+    ptr: *const c_char,
+    string_length_in_bytes: u32,
+    argument_name: &str,
+) -> Result<&str, CError> {
     try_ptr_as_ref(ptr, argument_name)?;
 
-    match unsafe { CStr::from_ptr(ptr) }.to_str() {
+    let byte_slice =
+        unsafe { std::slice::from_raw_parts(ptr.cast::<u8>(), string_length_in_bytes as usize) };
+
+    // Make sure there's no null-terminator within that range.
+    // We're strict and fail then because the input doesn't match our expectations.
+    // Alternatively, we could cut the string short at the nullterminator.
+    if let Some(null_terminator_position) = byte_slice.iter().position(|b| *b == b'\0') {
+        return Err(CError::new(
+            CErrorCode::InvalidStringArgument,
+            &format!("Argument {argument_name:?} was specified to be a string with length {string_length_in_bytes}, but there is an unexpected null-terminator at position {null_terminator_position}."),
+        ));
+    }
+
+    // The byte slice is
+    match std::str::from_utf8(byte_slice) {
         Ok(str) => Ok(str),
         Err(utf8_error) => Err(CError::invalid_str_argument(argument_name, utf8_error)),
     }