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

Introduce remaining Index traits #159

Closed
wants to merge 1 commit into from

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Jul 8, 2014

No description provided.

@lilyball
Copy link
Contributor

lilyball commented Jul 8, 2014

👍 Overall I think this is good.

What happens if T implements Index and &T implements IndexGet? Seems like an ambiguity right there. And if the type T implements both Index and IndexGet then it seems like potential confusion. Could the compiler perhaps forbid both these scenarios? Plus the case of T implementing IndexMut and &mut T implementing IndexGet.

@sfackler
Copy link
Member Author

sfackler commented Jul 8, 2014

Yeah, we'd probably want the compiler to forbid "overlapping" definitions.

fn index<'a>(&'a self, element: E) -> &'a R;
}

pub trait IndexMut<E, R>: Index<E, R> {
Copy link
Member

Choose a reason for hiding this comment

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

Why does IndexMut<E, R> require Index<E, R>?

Copy link
Member

Choose a reason for hiding this comment

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

For the upgrade mechanism required to fix the DerefMut-in-immutable-contexts issue.
Or is it downgrade? In any case, one of IndexMut or Index is picked, and depending on whether the result is required to be a mutable lvalue (e.g. method call with &mut self) or not, the operation will be adjusted to use the more appropriate trait.

@SiegeLord
Copy link

Why can't IndexSet have a by-value self like IndexGet does? It seems like you already lost the generic usability once you define IndexGet like that, so why not make IndexSet just as flexible?

My use case is a matrix type defined like so:

struct Matrix<T>
{
    elems: Vec<Cell<T>>
}

@erickt
Copy link

erickt commented Jul 17, 2014

I don't think we need IndexGet if we reformulate Index to keep the return type fully abstract:

extern crate collections;

use collections::Bitv;

trait MyIndex<'a, E, R> {
    fn my_index(&'a self, element: E) -> R;
}

impl<'a, T> MyIndex<'a, uint, &'a T> for &'a [T] {
    fn my_index(&'a self, element: uint) -> &'a T {
        &self[element]
    }
}

impl<'a> MyIndex<'a, uint, bool> for Bitv {
    fn my_index(&'a self, element: uint) -> bool {
        if self.get(element) {
            true
        } else {
            false
        }
    }
}

fn main() {
    let x = [1u, 2, 3];
    assert_eq!(x.my_index(0), &1u);
    assert_eq!(x.my_index(1), &2u);

    let mut x = Bitv::with_capacity(3, false);
    x.set(0, true);
    x.set(1, true);
    assert_eq!(x.my_index(0), true);
    assert_eq!(x.my_index(1), true);
    assert_eq!(x.my_index(2), false);
}

@sfackler
Copy link
Member Author

I'm assuming the lifetime parameter in MyIndex is unneeded? It would also need to take self by value to handle the index-move case.

This option is mentioned briefly in the RFC. @alexcrichton had some concerns about it with respect to how the compiler would decide what &mut foo[i] and &foo[i] would desugar to. I think we can probably make it work with the right restrictions to prevent ambiguously overlapping implementations of Index (though I'm not totally sure what those would be).

I am still a bit concerned with the weirdness of the by-value overload setup, but it's not the end of the world, and it seems to add a lot more flexibility.

@erickt
Copy link

erickt commented Jul 22, 2014

@sfackler: Ah, I apparently didn't understand that you meant in that section. I think if we go down my route, are there any situations where a data structure would want to have overlapping implementations of Index? The only one I can think of is for an optimization, as in: let v = vec!(1,2,3); let x = *v[1], but I bet llvm can already optimize that into an optimal form. If that's the case, I don't think it'd be that odd to have index return by-ref or by-value depending on the types. We already kind of do this with our iterators.

Also, the lifetime was needed, but with perhaps it's unnecessary now that we have lifetime elision?

@nikomatsakis
Copy link
Contributor

This is an important topic, but by now it's clear we won't be getting to this at the moment. I hope we can come back to it shortly after 1.0 though. Postponing along with DerefMove under issue #997 (I see both issues as related and would like to address them together).

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.

7 participants