-
-
Notifications
You must be signed in to change notification settings - Fork 249
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.
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
Added oids_of!
macro
#1879
base: develop
Are you sure you want to change the base?
Added oids_of!
macro
#1879
Conversation
pgrx/src/datum/mod.rs
Outdated
#[macro_export] | ||
macro_rules! oids_of { | ||
($($t:path),* $(,)?) => ({ | ||
vec![$(::pgrx::pg_sys::PgOid::from(<$t>::type_oid())),*] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should include a vec![]
when it could instead be invoked as vec![oids_of!(...)]
this would allow constructing arrays also, or other structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the plan when I get rid of Option<Vec<PgOid>>
because now it's too wordy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...if that's the plan, then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I will remove vec!
from here and will change the methods of SpiClient
to use slices as soon as you that the breaking change time has come (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not add this then just break it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let it be non-breaking then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this has the right semantics right now. It's not clear to me that oids_of!()
should be an InvalidOid
, instead of, say, a compile-time error. Or an empty array.
I think we should ship a non-plural |
If something isn't an Let's say, I don't want let _: &[i32] = oids_of![]; That's why there's |
You have it already, |
tbh, that's kind of the problem. A macro creates a false impression this will resolve statically. That fn produces runtime lookups. |
It depends, actually. For built-in types, it's static, but for custom types it isn't. |
I realize that for the existing inputs, it's somewhat incoherent to use. But the reason to make it a zero-sized array, effectively, is to make a macro-call to it... possibly itself from a macro-expansion... valid anywhere we ask for let oids = oids_of![$($types,)];
oids.is_empty().then_some(&oids) but this enables that. Basically, we shouldn't overfit it to the existing use-cases. |
Sorry, not following. Could you reword it a bit so I can get what I should do? |
That's an improvement for SPI making it less cumbersome. Just to feel the difference look at the tests. Who would really want to write something like
PgBuiltInOids::INT4OID.oid()
instead of just telling a Rust type?I will change the behavior of this macro a bit when you will be ready for shipping
0.13
, so it won't produce any vector but a slice.