Skip to content

Conversation

@Stebalien
Copy link
Contributor

Instead of using the O(n) defaults, define O(1) shortcuts. I also copied (and slightly modified) the relevant tests from the iter tests into the slice tests just in case someone comes along and changes them in the future.

Partially implements #24214.

Instead of using the O(n) defaults, define O(1) shortcuts.
@rust-highfive
Copy link
Contributor

r? @pcwalton

(rust_highfive has picked a reviewer for you, use r? to override)

@Stebalien
Copy link
Contributor Author

r? @aturon

@rust-highfive rust-highfive assigned aturon and unassigned pcwalton Apr 22, 2015
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like next_back is better: it seems like the write will often be eliminated anyway, and even if it isn't, it's cheap: i.e. the unsafe code isn't worth it.

1. Use next_back for last.
2. Use slices for computing nth. It might be possible to use the same
code for both the mut/const case but I don't know how that will play
with compiler optimizations.
Copy link
Member

Choose a reason for hiding this comment

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

The logic here (especially with 0-sized types) is unfortunately duplicated with the next functions, would it be possible to deduplicate these conditionals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up using a macro to work around the mut/const problem. I could have used custom traits but that would have been a lot of extra code for little functionality... Something like (untested):

trait Reference<T> {
    type Pointer;
    unsafe fn as_ptr(&self) -> Self::Pointer {
        self as Self::Pointer
    }
}

trait Pointer<T> {
    type Reference;
    unsafe fn as_ref(&self) -> Self::Reference {
        transmute(self)
    }
    unsafe fn as_slice_ref(&self) -> Self::Reference {
        if mem::size_of::<T>() == 0 {
            &mut *(1 as *mut _)
        } else {
            self.as_ref()
        }
    }
    unsafe fn slice_offset(n: isize) -> Self;
}

impl<T> Reference<T> for &T {
    type Pointer = *const T;
}

impl<T> Reference<T> for &mut T {
    type Pointer = *mut T;
}

impl<T> Pointer<T> for *const T {
    type Reference = &T;

    unsafe fn slice_offset(n: isize) -> Self {
        if mem::size_of::<T>() == 0 {
            transmute(self as usize + n)
        } else {
            self.offset(n)
        }
    }
}

impl<T> Pointer<T> for *mut T {
    type Reference = &mut T;

    unsafe fn slice_offset(n: isize) -> Self {
        if mem::size_of::<T>() == 0 {
            transmute(self as usize + n)
        } else {
            self.offset(n)
        }
    }
}

@Stebalien
Copy link
Contributor Author

Why do slice iterators return &mut *(1 as *mut) (references to 0x1) instead of the actual pointer into the slice (self.ptr)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% clear on why the slice_ref macro is needed here -- doesn't elem_ref already have the correct type and semantics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hence my question:

Why do slice iterators return &mut *(1 as *mut) (references to 0x1) instead of the actual pointer into the slice (self.ptr)?

elem_ref is the actual pointer but slice_ref! will convert it to &mut *(1 as *mut _) if the type is zero sized.

Copy link
Member

Choose a reason for hiding this comment

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

This may be a relic of an era since long past, I think elem_ref should be usable as-is today?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if making this an actual pointer will have a performance impact. As-is, iterators over zero-sized types always yield constants.

Copy link
Member

Choose a reason for hiding this comment

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

Hm that's true, I think it's fine to investigate this on the side though as the rest of this PR should be good to go, thanks @Stebalien!

@aturon
Copy link
Contributor

aturon commented Apr 27, 2015

Sorry for the delay reviewing this (please feel free to ping me on IRC, my inbox was recently a bit crazy and I was missing some things).

This PR looks good to me -- I left you a minor question, but otherwise I'm ready to send it to bors. Thanks for doing this!

@alexcrichton
Copy link
Member

@bors: r+ e129b92

@bors
Copy link
Collaborator

bors commented Apr 27, 2015

⌛ Testing commit e129b92 with merge 97d4e76...

bors added a commit that referenced this pull request Apr 27, 2015
Instead of using the O(n) defaults, define O(1) shortcuts. I also copied (and slightly modified) the relevant tests from the iter tests into the slice tests just in case someone comes along and changes them in the future.

Partially implements  #24214.
@bors
Copy link
Collaborator

bors commented Apr 28, 2015

@bors bors merged commit e129b92 into rust-lang:master Apr 28, 2015
@Stebalien Stebalien deleted the slice branch June 11, 2015 16:09
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.

9 participants