Add derive macros for Encode, Decode and HasSqlType for Transparent types, weak/strong enums and structs #97
Conversation
Freax13
requested review from
mehcode and
abonander
and removed request for
mehcode
|
I'm haven't gone through this in detail yet but it's looking pretty good so far, especially if it's passing CI! However, I'd like to see
|
|
Looked through everything except the |
|
From a quick glance, this looks aweomse @Freax13. I don't have time to give it an in-depth review and play test but I just want to check-in and let you know that we haven't forgotten about it. The plan is to get this in for 0.3. |
There was a problem hiding this comment.
Looking good so far, a few nits though. In general you're using Vec a lot where I think iterator chains would suffice, which introduces intermediate allocations that are probably unnecessary. In some places it's clear you're trying to avoid iterating something twice but that's probably going to be faster than allocation in the general case anyway (since we're dealing with such small datasets).
I'm wary of landing an interface leaning this heavily on OIDs though, even as an initial prototype, and especially without support for migrations. Type mismatches could give some really weird errors that would be a nightmare to debug. I could easily add a query for mapping type names to OIDs in #108 (I'm already doing the inverse there).
| let id = &v.ident; | ||
| parse_quote!(_ if (#ident :: #id as #repr) == val => Ok(#ident :: #id),) | ||
| }) | ||
| .collect::<Vec<Arm>>(); |
There was a problem hiding this comment.
You actually don't need to .collect() here, quote!() can consume the iterator directly.
sqlx-macros/src/derives/decode.rs
Outdated
| let name = id.to_string(); | ||
| value_arms.push(quote!(#name => Ok(#ident :: #id),)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Did this code not work as an iterator chain?
sqlx-macros/src/derives/decode.rs
Outdated
| names.push(id.clone().unwrap()); | ||
| let ty = &field.ty; | ||
| reads.push(parse_quote!( | ||
| if buf.len() < 8 { |
There was a problem hiding this comment.
The missing indentation really threw me off here. I guess cargo fmt didn't catch it because it's in a macro invocation.
sqlx-macros/src/derives/decode.rs
Outdated
| let mut names: Vec<Ident> = Vec::new(); | ||
| for field in fields { | ||
| let id = &field.ident; | ||
| names.push(id.clone().unwrap()); |
There was a problem hiding this comment.
You can make names a Vec<&Ident> so you don't have to clone here. The references should be able to live long enough.
| } | ||
| )) | ||
| } | ||
| Ok(tts) |
There was a problem hiding this comment.
Did you and @mehcode already discuss what this should do if the postgres feature isn't activated? I would think this should error.
There was a problem hiding this comment.
no, we did not discuss that yet
sqlx-macros/src/derives/encode.rs
Outdated
| let size = end - start; | ||
|
|
||
| // replaces zeros with actual length | ||
| buf[start-4..start].copy_from_slice(&(size as u32).to_be_bytes()); |
There was a problem hiding this comment.
Maybe the same thing here as with decode_struct_field().
sqlx-macros/src/derives/encode.rs
Outdated
| #(#writes)* | ||
| } | ||
| fn size_hint(&self) -> usize { | ||
| 4 + #column_count * (4 + 4) + #(#sizes)+* |
There was a problem hiding this comment.
I'd like a comment here explaining this arithmetic, for posterity.
| @@ -32,6 +33,10 @@ impl PgTypeInfo { | |||
| pub fn with_oid(oid: u32) -> Self { | |||
| Self { id: TypeId(oid) } | |||
| } | |||
|
|
|||
| pub fn oid(&self) -> u32 { | |||
There was a problem hiding this comment.
We'd probably rather not expose this in the public API if possible (with custom types we may not always have an OID so this will need to be changed to Option<u32>). Either pub(crate) or #[doc(hidden)].
| input, | ||
| "structs with zero or more than one unnamed field are not supported", | ||
| )), | ||
| Data::Struct(DataStruct { |
There was a problem hiding this comment.
I would prefer to sort all the Data::Struct arms together, then Data::Enum and then Data::Union.
| input, | ||
| "structs with zero or more than one unnamed field are not supported", | ||
| )), | ||
| Data::Struct(DataStruct { |
There was a problem hiding this comment.
Same thing re: match arm sorting now.
| input, | ||
| "structs with zero or more than one unnamed field are not supported", | ||
| )), | ||
| Data::Struct(DataStruct { |
|
Merged in with #134. Thanks again for the awesome contribution. |
this is an implementation for the ideas in #76