Split from_gatt and to_gatt into separate traits and implement for &'static str #289
Conversation
|
Just a thought, should the trait be IntoGatt rather than ToGatt? I'm not sure which is more idiomatic. |
I wondered this, and then thought that if the trait was called IntoGatt, the function should also be called into_gatt... EDIT: Turns out Clippy has a good answer: |
…ltd/trouble into fix-string-slice-impl-of-gattvalue
|
/test |
|
In reference to the trait names there is potentially an argument to be made that it should be This is a nit though so don't feel obligated at all |
| impl ToGatt for &'static str { | ||
| const MIN_SIZE: usize = 0; | ||
| const MAX_SIZE: usize = usize::MAX; | ||
|
|
||
| fn to_gatt(&self) -> &[u8] { | ||
| self.as_bytes() | ||
| } | ||
| } |
There was a problem hiding this comment.
Does this have to be static? I'd imagine almost any use case would have it be static but I wonder if you can get a little extra flexibility by using a generic lifetime. This should work the same for a &'static str I believe while also allowing non-static &strs to be used assuming the borrow checker agrees.
| impl ToGatt for &'static str { | |
| const MIN_SIZE: usize = 0; | |
| const MAX_SIZE: usize = usize::MAX; | |
| fn to_gatt(&self) -> &[u8] { | |
| self.as_bytes() | |
| } | |
| } | |
| impl<'a> ToGatt for &'a str { | |
| const MIN_SIZE: usize = 0; | |
| const MAX_SIZE: usize = usize::MAX; | |
| fn to_gatt(&self) -> &'a [u8] { | |
| self.as_bytes() | |
| } | |
| } |
There was a problem hiding this comment.
See the discussion here #272 @jscatena88
There was a problem hiding this comment.
It looks like the complications there were from implementing from_gatt not to_gatt. I'm still somewhat confident that the code I wrote above should work actually. It is passing tests locally on my laptop (except for one test that seems to be expecting connected hardware? gatt_client_server). But either way not a big deal
|
Overall looks like a great improvement! I don't have time at this exact moment to test on HW or on the project I was working on but I see no reason it wouldn't all work |
|
Just to mention, I'm fully on board with moving to AsGatt instead, I think there's a strong argument there. I'll raise a PR for it. |
The implementation of
GattValuefor&'_ strwas incorrect, and was returning the reference's memory on the stack rather than the slice it was pointing to. Furthermore, there were issues linking lifetimes of references as gatt values to their &[u8] equivalent (see #272 for more details)This PR splits
GattValueinto separate traitsFromGattandToGattwhereFromGattrequires thatToGattis implemented. This allows us to implementto_gatton&'static strand other static references without having to implementfrom_gatt, which would potentially be unsafe.Fixes #272.