Skip to content

Commit

Permalink
Rename RawPtr::to_option() to RawPtr::as_ref()
Browse files Browse the repository at this point in the history
As outlined in

  https://aturon.github.io/style/naming/conversions.html

`to_` functions names should only be used for expensive operations.
Thus `to_option` is better named `as_option`. Also, putting type
names into method names is considered bad style; what the user is
really trying to get is a reference. This `as_ref` is even better.

Also, we are missing a mutable version of this method. So add a
new trait `RawMutPtr` with a corresponding `as_mut` methode.

Finally, there is a bug in the signature of `to_option` which has
been around since lifetime elision: originally the returned reference
had 'static lifetime, but since the elision changes this become
the lifetime of the raw pointer (which does not make sense, since
the pointer lifetime and referent lifetime are unrelated). Fix
the bug to return a reference with a fresh lifetime (which will
be inferred from the calling context).

[breaking-change]
  • Loading branch information
apoelstra committed Aug 31, 2014
1 parent 27e8d5b commit 00ff5aa
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 15 deletions.
2 changes: 1 addition & 1 deletion src/libcollections/dlist.rs
Expand Up @@ -90,7 +90,7 @@ impl<T> Rawlink<T> {
/// Convert the `Rawlink` into an Option value
fn resolve_immut<'a>(&self) -> Option<&'a T> {
unsafe {
mem::transmute(self.p.to_option())
self.p.as_ref()
}
}

Expand Down
46 changes: 38 additions & 8 deletions src/libcore/ptr.rs
Expand Up @@ -256,27 +256,46 @@ pub unsafe fn position<T>(buf: *const T, f: |&T| -> bool) -> uint {
pub trait RawPtr<T> {
/// Returns the null pointer.
fn null() -> Self;

/// Returns true if the pointer is equal to the null pointer.
fn is_null(&self) -> bool;

/// Returns true if the pointer is not equal to the null pointer.
fn is_not_null(&self) -> bool { !self.is_null() }

/// Returns the value of this pointer (ie, the address it points to)
fn to_uint(&self) -> uint;
/// Returns `None` if the pointer is null, or else returns the value wrapped
/// in `Some`.

/// Returns `None` if the pointer is null, or else returns a reference to the
/// value wrapped in `Some`.
///
/// # Safety Notes
///
/// While this method is useful for null-safety, it is important to note
/// that this is still an unsafe operation because the returned value could
/// be pointing to invalid memory.
unsafe fn to_option(&self) -> Option<&T>;
/// While this method and its mutable counterpart are useful for null-safety,
/// it is important to note that this is still an unsafe operation because
/// the returned value could be pointing to invalid memory.
unsafe fn as_ref<'a>(&self) -> Option<&'a T>;

/// A synonym for `as_ref`, except with incorrect lifetime semantics
#[deprecated="Use `as_ref` instead"]
unsafe fn to_option<'a>(&'a self) -> Option<&'a T> {
mem::transmute(self.as_ref())
}

/// Calculates the offset from a pointer. The offset *must* be in-bounds of
/// the object, or one-byte-past-the-end. `count` is in units of T; e.g. a
/// `count` of 3 represents a pointer offset of `3 * sizeof::<T>()` bytes.
unsafe fn offset(self, count: int) -> Self;
}

/// Methods on mutable raw pointers
pub trait RawMutPtr<T>{
/// Returns `None` if the pointer is null, or else returns a mutable reference
/// to the value wrapped in `Some`. As with `as_ref`, this is unsafe because
/// it cannot verify the validity of the returned pointer.
unsafe fn as_mut<'a>(&self) -> Option<&'a mut T>;
}

impl<T> RawPtr<T> for *const T {
#[inline]
fn null() -> *const T { null() }
Expand All @@ -293,7 +312,7 @@ impl<T> RawPtr<T> for *const T {
}

#[inline]
unsafe fn to_option(&self) -> Option<&T> {
unsafe fn as_ref<'a>(&self) -> Option<&'a T> {
if self.is_null() {
None
} else {
Expand All @@ -318,7 +337,7 @@ impl<T> RawPtr<T> for *mut T {
}

#[inline]
unsafe fn to_option(&self) -> Option<&T> {
unsafe fn as_ref<'a>(&self) -> Option<&'a T> {
if self.is_null() {
None
} else {
Expand All @@ -327,6 +346,17 @@ impl<T> RawPtr<T> for *mut T {
}
}

impl<T> RawMutPtr<T> for *mut T {
#[inline]
unsafe fn as_mut<'a>(&self) -> Option<&'a mut T> {
if self.is_null() {
None
} else {
Some(&mut **self)
}
}
}

// Equality for pointers
impl<T> PartialEq for *const T {
#[inline]
Expand Down
35 changes: 30 additions & 5 deletions src/libcoretest/ptr.rs
Expand Up @@ -102,19 +102,44 @@ fn test_is_null() {
}

#[test]
fn test_to_option() {
fn test_as_ref() {
unsafe {
let p: *const int = null();
assert_eq!(p.to_option(), None);
assert_eq!(p.as_ref(), None);

let q: *const int = &2;
assert_eq!(q.to_option().unwrap(), &2);
assert_eq!(q.as_ref().unwrap(), &2);

let p: *mut int = mut_null();
assert_eq!(p.to_option(), None);
assert_eq!(p.as_ref(), None);

let q: *mut int = &mut 2;
assert_eq!(q.to_option().unwrap(), &2);
assert_eq!(q.as_ref().unwrap(), &2);

// Lifetime inference
let u = 2i;
{
let p: *const int = &u as *const _;
assert_eq!(p.as_ref().unwrap(), &2);
}
}
}

#[test]
fn test_as_mut() {
unsafe {
let p: *mut int = mut_null();
assert!(p.as_mut() == None);

let q: *mut int = &mut 2;
assert!(q.as_mut().unwrap() == &mut 2);

// Lifetime inference
let mut u = 2i;
{
let p: *mut int = &mut u as *mut _;
assert!(p.as_mut().unwrap() == &mut 2);
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/liblog/lib.rs
Expand Up @@ -282,7 +282,7 @@ impl Drop for DefaultLogger {
pub fn log(level: u32, loc: &'static LogLocation, args: &fmt::Arguments) {
// Test the literal string from args against the current filter, if there
// is one.
match unsafe { FILTER.to_option() } {
match unsafe { FILTER.as_ref() } {
Some(filter) if filter.is_match(args.to_string().as_slice()) => return,
_ => {}
}
Expand Down

5 comments on commit 00ff5aa

@bors
Copy link
Contributor

@bors bors commented on 00ff5aa Sep 3, 2014

Choose a reason for hiding this comment

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

saw approval from aturon
at apoelstra@00ff5aa

@bors
Copy link
Contributor

@bors bors commented on 00ff5aa Sep 3, 2014

Choose a reason for hiding this comment

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

merging apoelstra/rust/to-option-fix = 00ff5aa into auto

@bors
Copy link
Contributor

@bors bors commented on 00ff5aa Sep 3, 2014

Choose a reason for hiding this comment

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

apoelstra/rust/to-option-fix = 00ff5aa merged ok, testing candidate = 6ac4a30

@bors
Copy link
Contributor

@bors bors commented on 00ff5aa Sep 3, 2014

Choose a reason for hiding this comment

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

fast-forwarding master to auto = 6ac4a30

Please sign in to comment.