Skip to content

Commit

Permalink
Clean up library with is_non_null (#1528)
Browse files Browse the repository at this point in the history
Adds `PointerExt::is_non_null` and cleans up some library code.
This route was chosen because expressions like the following:
```rust
if !(*(*some_ptr).field.another_ptr_field).something_else.is_null() {
```
make it easy to lose track of the `!` by the time you arrive at `is_null`,
thus making it very easy to confuse the usage of negation.
`null_mut` is odd anyways, easily confusable from the "C view" with some
sort of `void*`, and Postgres has many `bool* nulls`, too.
  • Loading branch information
workingjubilee committed Feb 5, 2024
1 parent 395c975 commit 0e01d93
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 8 deletions.
1 change: 1 addition & 0 deletions pgrx/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ pub use once_cell;

/// Not ready for public exposure.
mod layout;
mod ptr;
mod slice;
mod toast;

Expand Down
9 changes: 5 additions & 4 deletions pgrx/src/list/flat_list.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::{Enlist, List, ListCell, ListHead};
use crate::memcx::MemCx;
use crate::pg_sys;
use crate::ptr::PointerExt;
use crate::seal::Sealed;
use core::cmp;
use core::ffi;
Expand Down Expand Up @@ -195,7 +196,7 @@ impl<'cx, T: Enlist> List<'cx, T> {
};

// Remember to check that our raw ptr is non-null
if raw != ptr::null_mut() {
if raw.is_non_null() {
// Shorten the list to prohibit interaction with List's state after drain_start.
// Note this breaks List repr invariants in the `drain_start == 0` case, but
// we only consider returning the list ptr to `&mut self` if Drop is completed
Expand Down Expand Up @@ -332,11 +333,11 @@ unsafe fn grow_list(list: &mut pg_sys::List, target: usize) {
if list.elements == ptr::addr_of_mut!(list.initial_elements).cast() {
// first realloc, we can't dealloc the elements ptr, as it isn't its own alloc
let context = pg_sys::GetMemoryChunkContext(list as *mut pg_sys::List as *mut _);
if context == ptr::null_mut() {
if context.is_null() {
panic!("Context free list?");
}
let buf = pg_sys::MemoryContextAlloc(context, alloc_size);
if buf == ptr::null_mut() {
if buf.is_null() {
panic!("List allocation failure");
}
ptr::copy_nonoverlapping(list.elements, buf.cast(), list.length as _);
Expand Down Expand Up @@ -382,7 +383,7 @@ pub struct Drain<'a, 'cx, T> {

impl<'a, 'cx, T> Drop for Drain<'a, 'cx, T> {
fn drop(&mut self) {
if self.raw == ptr::null_mut() {
if self.raw.is_null() {
return;
}

Expand Down
7 changes: 4 additions & 3 deletions pgrx/src/list/linked_list.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use super::{Enlist, List, ListCell, ListHead};
use crate::memcx::MemCx;
use crate::pg_sys;
use crate::ptr::PointerExt;
use crate::seal::Sealed;
use core::cmp;
use core::ffi;
Expand Down Expand Up @@ -209,7 +210,7 @@ impl<'cx, T: Enlist> List<'cx, T> {
};

// Remember to check that our raw ptr is non-null
if raw != ptr::null_mut() {
if raw.is_non_null() {
// Shorten the list to prohibit interaction with List's state after drain_start.
// Note this breaks List repr invariants in the `drain_start == 0` case, but
// we only consider returning the list ptr to `&mut self` if Drop is completed
Expand Down Expand Up @@ -328,7 +329,7 @@ unsafe fn cons_cell<T: Enlist>(list: &mut pg_sys::List, value: T) -> *mut pg_sys

unsafe fn destroy_list(list: *mut pg_sys::List) {
let mut cell = (*list).head;
while cell != ptr::null_mut() {
while cell.is_non_null() {
let next = (*cell).next;
pg_sys::pfree(cell.cast());
cell = next;
Expand Down Expand Up @@ -496,7 +497,7 @@ impl<T: Enlist> Iterator for RawCellIter<T> {

#[inline]
fn next(&mut self) -> Option<T> {
if self.ptr != ptr::null_mut() {
if self.ptr.is_non_null() {
let ptr = self.ptr;
// SAFETY: It's assumed that the pointers are valid on construction
unsafe {
Expand Down
3 changes: 2 additions & 1 deletion pgrx/src/pg_sys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

// Flatten out the contents into here.
use crate::memcx;
use crate::ptr::PointerExt;
pub use pgrx_pg_sys::*;

// Interposing here can allow extensions like ZomboDB to skip the cshim,
Expand Down Expand Up @@ -39,7 +40,7 @@ pub unsafe fn rt_fetch(index: Index, range_table: *mut List) -> *mut RangeTblEnt
#[inline]
pub unsafe fn planner_rt_fetch(index: Index, root: *mut PlannerInfo) -> *mut RangeTblEntry {
unsafe {
if (*root).simple_rte_array != core::ptr::null_mut() {
if (*root).simple_rte_array.is_non_null() {
*(*root).simple_rte_array.add(index as _)
} else {
rt_fetch(index, (*(*root).parse).rtable).cast()
Expand Down
15 changes: 15 additions & 0 deletions pgrx/src/ptr.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
pub(crate) trait PointerExt {
fn is_non_null(&self) -> bool;
}

impl<T> PointerExt for *mut T {
fn is_non_null(&self) -> bool {
!self.is_null()
}
}

impl<T> PointerExt for *const T {
fn is_non_null(&self) -> bool {
!self.is_null()
}
}

0 comments on commit 0e01d93

Please sign in to comment.