Fix potential issue discovered by new lint suspicious_to_owned#49
Fix potential issue discovered by new lint suspicious_to_owned#49pawelchcki wants to merge 4 commits into
Conversation
pawelchcki
force-pushed
the
pawel/fix_to_owned
branch
from
b71aeda to
a865499
Compare
| 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()); |
There was a problem hiding this comment.
As it stands right now into_owned() was a noop, so I think we can remove it completely
There was a problem hiding this comment.
I'm not sure why we were doing clone + to_owned. I might be missing something.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
|
Thanks, Pawel. I forgot this was already open when I opened #52, which superseded this one. |
Co-authored-by: Ivo Anjo <ivo.anjo@datadoghq.com>
4 participants
see:
rust-lang/rust-clippy#8984