-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
Switch to ManuallyDropUnion
#1547
Switch to ManuallyDropUnion
#1547
Conversation
Pull in changes from pgcentralfoundation/pgrx
let varattrib_4b: *mut _ = | ||
&mut varlena.cast::<pg_sys::varattrib_4b>().as_mut().unwrap_unchecked().va_4byte; |
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.
Hm, okay... so we cast the varlena pointer, then call as_mut()
, then unwrap_unchecked
it, then deref .va_4byte
... since we're just derefing anyways, can't this just be
&mut (*varlena.cast::<pg_sys::varattrib_4b>()).va_4byte;
or something like that?
...also why is it a pointer? If that is important for soundness, this should actually be written with core::ptr::addr_of_mut!
without &mut
or as_mut
. I think when I first reviewed this code, I didn't have my feet under me on the exact semantics of place expressions the way I do now: we want to create a place expression without expressing the strong semantics of &mut
, and get a pointer to that place, which is what the macro I mentioned allows.
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.
This should be good after a little bit of cleanup.
Before:
#[repr(C)]
pub struct varattrib_4b {
pub va_4byte: __BindgenUnionField<varattrib_4b__bindgen_ty_1>,
pub va_compressed: __BindgenUnionField<varattrib_4b__bindgen_ty_2>,
pub bindgen_union_field: [u32; 2usize],
}
After:
#[repr(C)]
pub union varattrib_4b {
pub va_4byte: ::std::mem::ManuallyDrop<varattrib_4b__bindgen_ty_1>,
pub va_compressed: ::std::mem::ManuallyDrop<varattrib_4b__bindgen_ty_2>,
}
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.
Great!
Welcome to pgrx 0.12.0-alpha.1! Say the magic words with me! ```shell cargo install cargo-pgrx --locked --version 0.12.0-alpha.1 ``` # Breaking Changes ## No more dlopen! Perhaps the most exciting change this round is @usamoi's contribution in #1468 which means that we no longer perform a `dlopen` in order to generate the schema. The cost, such as it is, is that your pgrx extensions now require a `src/bin/pgrx_embed.rs`, which will be used to generate the schema. This has much less cross-platform issues and will enable supporting things like `cargo binstall` down the line. It may be a bit touchy on first-time setup for transitioning older repos. If necessary, you may have to directly add a `src/bin/pgrx_embed.rs` and add the following code (which should be the only code in the file, though you can add comments if you like?): ```rust ::pgrx::pgrx_embed!(); ``` Your Cargo.toml will also want to update its crate-type key for the library: ```toml [lib] crate-type = ["cdylib", "lib"] ``` ## Library Code - pgrx-pg-sys will now use `ManuallyDropUnion` thanks to @NotGyro in #1547 - VARHDRSZ `const`s are no longer `fn`, thanks to @workingjubilee in #1584 - We no longer have `Interval::is_finite` since #1594 - We translate more `*_tree_walker` functions to the same signature their `*_impl` version in Postgres 16 has: #1596 - Thanks to @eeeebbbbrrrr in #1591 we no longer have the `pg_sql_graph_magic!()` macro, which should help with more things in the future! # What's New We have quite a lot of useful additions to our API: - `SpiClient::prepare_mut` was added thanks to @XeniaLu in #1275 - @usamoi also contributed bindings subscripting code in #1562 - For `#[pg_test]`, you have been able to use `#[should_panic(expected = "string")]` to anticipate a panic that contains that string in that test. For various reasons, `#[pg_test(error = "string")]` is much the same. Now, you can also use `#[pg_test(expected = "string")]`, in the hopes that is easier to stumble across, as of #1570 ## `Result<composite_type!("..."), E>` support - In #1560 @NotGyro contributed support for using `Result<composite_type!("Name"), E>`, as a case that had not been handled before. ## Significantly expanded docs Thanks to @rjuju, @NotGyro, and @workingjubilee, we now have significantly expanded docs for cargo-pgrx and pgrx in general. Some of these are in the API docs on https://docs.rs or the READMEs, but there's also a guide, now! It's not currently published, but is available as an [mdbook](https://github.com/rust-lang/mdBook) in the repo. Some diagnostic information that is also arguably documentation, like comments and the suggestion to `cargo install`, have also been improved, thanks to @workingjubilee in - #1579 - #1573 ## `#[pg_cast]` An experimental macro for a `CREATE CAST` was contributed by @xwkuang5 in #1445! ## Legal Stuff Thanks to @the-kenny in #1490 and @workingjubilee in #1504, it was brought to our attention that some dependencies had unusual legal requirements. So we fixed this with CI! We now check our code included into pgrx-using binaries is MIT/Apache 2.0 licensed, as is common across crates.io, using `cargo deny`!. The build tools will have more flexible legal requirements (partly due to the use of Mozilla Public License code in rustls). # Internal Changes Many internal cleanups were done thanks to - @workingjubilee in too many PRs to count! - @thomcc found a needless condition in #1501 - @nyurik in too many PRs to count! In particular: - we now actually `pfree` our `Array`s we detoasted as-of #1571 - creating a `RawArray` is now low-overhead due to #1587 ## Soundness Fixes We had a number of soundness issues uncovered or have added more tests to catch them. - Bounds-checking debug assertions for array access by @NotGyro in #1514 - Fix unsound `&` and `&mut` in `fcinfo.rs` by @workingjubilee in #1595 ## Less Deps Part of the cleanup by @workingjubilee was reducing the number of deps we compile: * cargo-pgrx: reduce trivial dep usages in #1499 * Update 2 syn in #1557 Hopefully it will reduce compile time and disk usage! ## New Contributors * @the-kenny made their first contribution in #1490 * @xwkuang5 made their first contribution in #1445 * @rjuju made their first contribution in #1516 * @nyurik made their first contribution in #1533 * @NotGyro made their first contribution in #1514 * @XeniaLu made their first contribution in #1275 **Full Changelog**: v0.12.0-alpha.0...v0.12.0-alpha.1
Out of the available methods of wrapping C unions in Bindgen,
__BindgenUnionField
generates code with questionable correctness. This was spotted by @workingjubilee in #1465. This pull request adds a.default_non_copy_union_style(NonCopyUnionStyle::ManuallyDrop)
line topgrx-pg-sys
'sbuild.rs
. Since the semantics around these types are different, I have also changed some methods to work with the new type signature (particularlyset_varsize_4b()
).