`tor_bytes::Readable` returning `.clone()` alternatives
This is regarding the comment/to-do within the
tor-bytes/src/impls.rs
file, line 83
commit hash 51d91508
...
// XXXX I wish I didn't have to clone this.
... and opens a discussion on available alternatives.
Option 1
Use .to_owned()
instead of .clone()
diff --git a/tor-bytes/src/impls.rs b/tor-bytes/src/impls.rs
index ebc6e03..33e8d5b 100644
--- a/tor-bytes/src/impls.rs
+++ b/tor-bytes/src/impls.rs
@@ -81,7 +81,7 @@ where
v.push(T::take_from(b)?);
}
// XXXX I wish I didn't have to clone this.
- Ok(Self::from_slice(v.as_slice()).clone())
+ Ok(Self::from_slice(v.as_slice()).to_owned())
}
}
Above usage of .to_owned()
offers more flexibility, is more explicit of
intent, and leaves implementation up to each data structure. But still will
result in calling .clone()
more often than not.
In short this may not solve anything or improve execution.
References
Option 2
Define explicit lifetime annotation
diff --git a/tor-bytes/src/impls.rs b/tor-bytes/src/impls.rs
index ebc6e03..a8c39bc 100644
--- a/tor-bytes/src/impls.rs
+++ b/tor-bytes/src/impls.rs
@@ -75,13 +75,13 @@ where
T: Readable + Clone,
N: generic_array::ArrayLength<T>,
{
- fn take_from(b: &mut Reader<'_>) -> Result<Self> {
+ fn take_from<'a>(b: &mut Reader<'_>) -> Result<&'a Self> {
let mut v: Vec<T> = Vec::new();
for _ in 0..N::to_usize() {
v.push(T::take_from(b)?);
}
// XXXX I wish I didn't have to clone this.
- Ok(Self::from_slice(v.as_slice()).clone())
+ Ok(Self::from_slice(v.as_slice()))
}
}
The above 'a
annotation hints to the borrow checker that returned result
should outlive the function scope, and completely removes need of .clone()
or .to_owned()
. But would require more rewriting than shown to include
lifetime hints elsewhere.
Personally I feel this to be the more of a solution. And because of the number of edits required to implement, if this option is chosen, then I'm willing to submit a Merge Request with changes.
References
Option 3
Use from_exact_iter
instead of from_slice
diff --git a/tor-bytes/src/impls.rs b/tor-bytes/src/impls.rs
index ebc6e03..b29794f 100644
--- a/tor-bytes/src/impls.rs
+++ b/tor-bytes/src/impls.rs
@@ -75,13 +75,15 @@ where
T: Readable + Clone,
N: generic_array::ArrayLength<T>,
{
+
fn take_from(b: &mut Reader<'_>) -> Result<Self> {
let mut v: Vec<T> = Vec::new();
for _ in 0..N::to_usize() {
v.push(T::take_from(b)?);
}
- // XXXX I wish I didn't have to clone this.
- Ok(Self::from_slice(v.as_slice()).clone())
+ // NOTE: `ok_or_else` may be a better choice for converting `Option`
+ // to `Result`. And `Error` type may need adjustment
+ Self::from_exact_iter(v).ok_or(Error::Internal)
}
}
This avoids the need of clone()
or to_owned()
because from_exact_iter()
returns an instance of GenericArray
instead of a borrow/reference. One big
drawback is much of from_exact_iter()
internal code is wrapped in an
unsafe block.
References