Skip to content

Fix potential issue discovered by new lint suspicious_to_owned #49

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

Closed
wants to merge 4 commits into from
Closed

Fix potential issue discovered by new lint suspicious_to_owned #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
230391f
Fix potential issue discovered by new lint suspicious_to_owned
pawelchcki Sep 5, 2022
a865499
use correct into_owned
pawelchcki Sep 5, 2022
3cf7abe
Remove Tag::into_owned() which currently is a noop
pawelchcki Sep 5, 2022
331586d
Use Iter::cloned() to copy the contents of iterator
pawelchcki Sep 5, 2022
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
5 changes: 0 additions & 5 deletions ddcommon/src/tag.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,6 @@ impl Tag {

Tag::from_value(format!("{}:{}", key, value))
}

pub fn into_owned(mut self) -> Self {
self.value = self.value.to_owned();
self
}
}

/// Parse a string of tags typically provided by environment variables
Expand Down
2 changes: 1 addition & 1 deletion profiling-ffi/src/exporter.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ pub extern "C" fn profile_exporter_new(
match || -> anyhow::Result<ProfileExporter> {
let family = unsafe { family.to_utf8_lossy() }.into_owned();
let converted_endpoint = unsafe { try_to_endpoint(endpoint)? };
let tags = tags.map(|tags| tags.iter().map(|tag| tag.clone().into_owned()).collect());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it stands right now into_owned() was a noop, so I think we can remove it completely

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why we were doing clone + to_owned. I might be missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's important that we get a Cow::Owned version because the tag can come from FFI and we don't know its real lifetime. I don't think clone will actually work for what we want:

https://doc.rust-lang.org/src/alloc/borrow.rs.html#194-203

That does say it just borrows if it's borrowed, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

The version before didn't do that though.

the Tag::into_owned was calling to Cow::to_owned which comes from the blanket implementation https://doc.rust-lang.org/std/borrow/enum.Cow.html#impl-ToOwned that just calls clone. So borrowed tags stayed borrowed before.

But I'm not sure why this is an issue, since tag have to be dropped using drop_tag which understands wether the cow is owned or borrowed?

let tags = tags.map(|tags| tags.iter().cloned().collect());
ProfileExporter::new(family, tags, converted_endpoint)
}() {
Ok(exporter) => NewProfileExporterResult::Ok(Box::into_raw(Box::new(exporter))),
Expand Down
2 changes: 1 addition & 1 deletion profiling/src/exporter/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ impl ProfileExporter {
form.add_text("version", "3");
form.add_text("start", start.format("%Y-%m-%dT%H:%M:%S%.9fZ").to_string());
form.add_text("end", end.format("%Y-%m-%dT%H:%M:%S%.9fZ").to_string());
form.add_text("family", self.family.to_owned());
form.add_text("family", self.family.clone());

for tags in self.tags.as_ref().iter().chain(additional_tags.iter()) {
for tag in tags.iter() {
Expand Down