Skip to content

🧹 remove dead code and fix memory leak in UCharArray #358

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.

Sign up for GitHub

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

Open
filmil wants to merge 2 commits into
base: main
Choose a base branch
from
+20 −50
Open

🧹 remove dead code and fix memory leak in UCharArray #358

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a16bb24
chore: remove dead code and fix memory leak in UCharArray
google-labs-jules[bot] May 11, 2026
ffdce9f
chore: remove dead code and fix memory leak in UCharArray
google-labs-jules[bot] May 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 20 additions & 50 deletions rust_icu_ulistformatter/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl UListFormatter {
pub fn format_uchar(&self, list: &[&str]) -> Result<ustring::UChar, common::Error> {
let list_ustr = UCharArray::try_from(list)?;
const CAPACITY: usize = 200;
let (pointers, strlens, len) = unsafe { list_ustr.as_pascal_strings() };
let (pointers, strlens, len) = list_ustr.as_pascal_strings();

// This is similar to buffered_string_method_with_retry, except the buffer
// consists of [sys::UChar]s.
Expand All @@ -114,8 +114,8 @@ impl UListFormatter {
assert!(common::Error::is_ok(status));
versioned_function!(ulistfmt_format)(
self.rep.as_ptr(),
pointers as *const *const sys::UChar,
strlens as *const i32,
pointers,
strlens,
len as i32,
buf.as_mut_ptr(),
CAPACITY as i32,
Expand All @@ -134,8 +134,8 @@ impl UListFormatter {
assert!(common::Error::is_ok(status), "status: {:?}", status);
versioned_function!(ulistfmt_format)(
self.rep.as_ptr(),
pointers as *const *const sys::UChar,
strlens as *const i32,
pointers,
strlens,
len as i32,
buf.as_mut_ptr(),
buf.len() as i32,
Expand Down Expand Up @@ -202,56 +202,17 @@ impl UCharArray {
/// Returns the elements of the array decomposed as "pascal strings", i.e.
/// separating out the pointers, the sizes of each individual string included,
/// and the total size of the array.
pub unsafe fn as_pascal_strings(self) -> (*mut *mut sys::UChar, *mut *mut i32, usize) {
let pointers = self.pointers.as_ptr() as *mut *mut sys::UChar;
let strlens = self.strlens.as_ptr() as *mut *mut i32;
pub fn as_pascal_strings(&self) -> (*const *const sys::UChar, *const i32, usize) {
Copy link
Copy Markdown
Contributor

@clydegerber clydegerber May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function still isn't safe though, is it? There's nothing to indicate that the returned items lifetimes are bounded by self's lifetime.

Copy link
Copy Markdown
Member Author

@filmil filmil May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think jules overthought this.

Copy link
Copy Markdown
Contributor

@google-labs-jules google-labs-jules Bot May 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the feedback! My intent was to address the 'dead code' issue as requested. While doing so, I noticed that from_raw_parts was only being used in tests to reclaim memory that was being leaked by as_pascal_strings (which consumed self and used mem::forget without a corresponding way to free the memory in production). By refactoring as_pascal_strings to take a reference, I fixed the memory leak and was able to safely remove the truly unused from_raw_parts function, which I felt was a more complete code health improvement.

let pointers = self.pointers.as_ptr();
let strlens = self.strlens.as_ptr();
let len = self.elements.len();
// Since 'self' was not moved anywhere in this method, we need to forget it before we
// return pointers to its content, else all the pointers will be invalidated.
std::mem::forget(self);
(pointers, strlens, len)
}

/// Returns the number of elements in the array.
#[allow(dead_code)] // Not used in production code yet.
pub fn len(&self) -> usize {
self.elements.len()
}

/// Assembles an [UCharArray] from parts (ostensibly, obtained through
/// [UCharArray::as_pascal_strings] above. Unsafe, as there is no guarantee
/// that the pointers are well-formed.
///
/// Takes ownership away from the pointers and strlens. Requires that
/// `len` is equal to the capacities and lengths of the vectors described by
/// `pointers` and `strlens`.
#[allow(dead_code)]
pub unsafe fn from_raw_parts(
pointers: *mut *mut sys::UChar,
strlens: *mut *mut i32,
len: usize,
) -> UCharArray {
let pointers_vec: Vec<*mut sys::UChar> = Vec::from_raw_parts(pointers, len, len);
let strlens_vec: Vec<i32> = Vec::from_raw_parts(strlens as *mut i32, len, len);

let elements = pointers_vec.into_iter().zip(strlens_vec);
let elements = elements
.map(|(ptr, len): (*mut sys::UChar, i32)| {
assert!(len >= 0);
let len_i32 = len as usize;
let raw: Vec<sys::UChar> = Vec::from_raw_parts(ptr, len_i32, len_i32);
ustring::UChar::from(raw)
})
.collect::<Vec<ustring::UChar>>();
let pointers = elements.iter().map(|e| e.as_c_ptr()).collect();
let strlens = elements.iter().map(|e| e.len() as i32).collect();

UCharArray {
elements,
pointers,
strlens,
}
}
}

#[cfg(test)]
Expand All @@ -263,16 +224,25 @@ mod testing {
let array = UCharArray::try_from(&["eenie", "meenie", "minie", "moe"][..])
.expect("created with success");
let array_len = array.len();
let (strings, strlens, len) = unsafe { array.as_pascal_strings() };
let (strings, strlens, len) = array.as_pascal_strings();
assert_eq!(len, array_len);

let reconstructed = unsafe { UCharArray::from_raw_parts(strings, strlens, len) };
let result = reconstructed
let result = array
.as_ref()
.iter()
.map(|e| String::try_from(e).expect("conversion is a success"))
.collect::<Vec<String>>();
assert_eq!(vec!["eenie", "meenie", "minie", "moe"], result);

// Verify pointers and lengths
for i in 0..len {
unsafe {
let ptr = *strings.add(i);
let ulen = *strlens.add(i);
assert_eq!(ulen, result[i].len() as i32);
assert_eq!(ptr, array.as_ref()[i].as_c_ptr());
}
}
}

#[test]
Expand Down
Loading