Skip to content
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

Paint bikeshed for Datum::cast_mut_ptr #702

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Sep 22, 2022

Before 0.5.0 goes gold, paint a bikeshed:

  • With pointer::cast_{const, mut} stabilizing in 1.65.0,Datum::ptr_cast seems off. Instead, follow the ordering implied by cast_mut, as_mut, and as_mut_ptr: cast, mutability, then _ptr or rarely _ref for pointer-kind. This gives us Datum::cast_mut_ptr::<T> instead.
  • Datum::to_void was used too rarely to warrant a unique fn, and cast_mut_ptr is more correct in 2 of those cases. Drop it.

Also, add some further verbiage about reference safety.

Before 0.5.0 goes gold, paint a bikeshed:
- With `pointer::cast_{const, mut}` stabilizing in 1.65.0,
  `Datum::ptr_cast` seems off. Instead, follow the ordering implied by
  `cast_mut`, `as_mut`, and `as_mut_ptr`:
  cast, mutability, then `_ptr` or rarely `_ref` for pointer-kind.
  This gives us `Datum::cast_mut_ptr::<T>` instead.
- `Datum::to_void` was used too rarely to warrant a unique fn,
  and `cast_mut_ptr` is more correct in 2 of those cases. Drop it.
This reverts commit c1d9fc9.
Not actually unused, it turns out.
/// True if the datum is equal to the null pointer.
pub fn is_null(self) -> bool {
self.0.is_null()
}

/// Assume the datum is a pointer and cast it to point to T.
pub fn ptr_cast<T>(self) -> *mut T {
/// It is recommended to explicitly use `datum.cast_mut_ptr::<T>()`.
pub fn cast_mut_ptr<T>(self) -> *mut T {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be useful to work like Arc::clone() and require the user to do T::cast_mut_ptr(thing)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's too high an impedance in this case, as normal pointer functions don't require it, and there's a very specific reason Arc requires that for certain functions:

Deref behavior

Arc automatically dereferences to T (via the Deref trait), so you can call T’s methods on a value of type Arc. To avoid name clashes with T’s methods, the methods of Arc itself are associated functions, called using fully qualified syntax:

Note this is not actually required for Arc::clone:

Arc’s implementations of traits like Clone may also be called using fully qualified syntax. Some people prefer to use fully qualified syntax, while others prefer using method-call syntax.

@Hoverbear
Copy link
Contributor

Is there a way for users to get the old functionality of Datum::to_void in the new way? If so, can we make sure to document it in the changelog?

@workingjubilee
Copy link
Member Author

Is there a way for users to get the old functionality of Datum::to_void in the new way? If so, can we make sure to document it in the changelog?

Yes. datum.cast_mut_ptr::<core::ffi::c_void>()

@workingjubilee workingjubilee merged commit fdbbce0 into pgcentralfoundation:develop Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants