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

Convert index operator to be by value #23601

Merged
merged 4 commits into from
Mar 24, 2015

Conversation

nikomatsakis
Copy link
Contributor

This is a [breaking-change]. When indexing a generic map (hashmap, etc) using the [] operator, it is now necessary to borrow explicitly, so change map[key] to map[&key] (consistent with the get routine). However, indexing of string-valued maps with constant strings can now be written map["abc"].

r? @japaric
cc @aturon @gankro

@nikomatsakis
Copy link
Contributor Author

(Hmm, something seems to have broken in the rebase. It WAS passing make check. I'll fix it up tomorrow.)

@nikomatsakis
Copy link
Contributor Author

OK, fixed the problem I was seeing locally -- rerunning make check now.

@nikomatsakis
Copy link
Contributor Author

oh and cc @sfackler

@Gankra
Copy link
Contributor

Gankra commented Mar 22, 2015

I'm not sure where we landed in discussion before. Longterm what's the strategy for recovering the old behaviour?

@nikomatsakis
Copy link
Contributor Author

ok, make check passes.

@nikomatsakis
Copy link
Contributor Author

@gankro if we want to recover the map[key], vs map[&key], there are two options:

  1. Optional autoref, probably applied uniformly to all by-value operators (that is, also to + and friends). (*)
  2. Extending coherence in such a way that we can provide two impls.

I would prefer option 1 since I think we want to have optional auto-ref for +, and I think that the more we can make the operators act uniformly the better. (I probably want to find ways to extend coherence too, but that's a complicated story...)

(All that said, my experience has been that I find the autoref on map[key] rather confusing. I can never remember whether I need an & or not, basically -- I expect map.get(&key) and map[&key] to act the same, but for the Option return type.)

(*) This may seem inconsistent with our conversation on IRC, since I think maybe (in retrospect) you were asking me about the prospects for "autoref" and I was saying "that's hard". There are indeed sometimes challenges about this sort of thing from inference; we'd have to see how well it works. But I think it'll work out ok in most cases, I guess.

@japaric
Copy link
Member

japaric commented Mar 22, 2015

@bors: r+ bae844d

@bors
Copy link
Contributor

bors commented Mar 22, 2015

☔ The latest upstream changes (presumably #23361) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis
Copy link
Contributor Author

@bors r=japaric b760c56

@nikomatsakis
Copy link
Contributor Author

@bors p=1

important language change

@bors
Copy link
Contributor

bors commented Mar 23, 2015

⌛ Testing commit b760c56 with merge 2aa6858...

bors added a commit that referenced this pull request Mar 23, 2015
This is a [breaking-change]. When indexing a generic map (hashmap, etc) using the `[]` operator, it is now necessary to borrow explicitly, so change `map[key]` to `map[&key]` (consistent with the `get` routine). However, indexing of string-valued maps with constant strings can now be written `map["abc"]`.

r? @japaric
cc @aturon @gankro
@bors
Copy link
Contributor

bors commented Mar 23, 2015

💔 Test failed - auto-mac-64-nopt-t

@nikomatsakis
Copy link
Contributor Author

@bors: retry

this test failure makes no sense!

@bors
Copy link
Contributor

bors commented Mar 23, 2015

☔ The latest upstream changes (presumably #23593) made this pull request unmergeable. Please resolve the merge conflicts.

references. For collections whose keys are integers, we take both
references and by-value.
`[]` on maps to `get` in rustc, since stage0 and stage1+ disagree about
how to use `[]`.
@nikomatsakis
Copy link
Contributor Author

@bors r=japaric 57cf2de p=1

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 23, 2015
This is a [breaking-change]. When indexing a generic map (hashmap, etc) using the `[]` operator, it is now necessary to borrow explicitly, so change `map[key]` to `map[&key]` (consistent with the `get` routine). However, indexing of string-valued maps with constant strings can now be written `map["abc"]`.

r? @japaric
cc @aturon @gankro
@bors
Copy link
Contributor

bors commented Mar 24, 2015

⌛ Testing commit 57cf2de with merge beb33d4...

@bors
Copy link
Contributor

bors commented Mar 24, 2015

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Mar 24, 2015

⌛ Testing commit 57cf2de with merge ffd5f00...

@bors
Copy link
Contributor

bors commented Mar 24, 2015

⛄ The build was interrupted to prioritize another pull request.

@bors
Copy link
Contributor

bors commented Mar 24, 2015

⌛ Testing commit 57cf2de with merge de8a23b...

bors added a commit that referenced this pull request Mar 24, 2015
This is a [breaking-change]. When indexing a generic map (hashmap, etc) using the `[]` operator, it is now necessary to borrow explicitly, so change `map[key]` to `map[&key]` (consistent with the `get` routine). However, indexing of string-valued maps with constant strings can now be written `map["abc"]`.

r? @japaric
cc @aturon @gankro
@bors
Copy link
Contributor

bors commented Mar 24, 2015

💔 Test failed - auto-linux-64-opt

@nikomatsakis
Copy link
Contributor Author

@bors: retry

@bors bors merged commit 57cf2de into rust-lang:master Mar 24, 2015
@nikomatsakis nikomatsakis deleted the by-value-index branch March 30, 2016 16:12
bors added a commit that referenced this pull request Mar 29, 2019
remove ?Sized bounds from Index

I've noticed that we have an `Idx: ?Sized` bound on the **index** in the `Index`, which seems strange given that we accept index by value. My guess is that it was meant to be removed in #23601, but was overlooked.

If I remove this bound, `./x.py src/libstd/ src/libcore/` passes, which means at least that this is not covered by test.

I think there's three things we can do here:

* run crater with the bound removed to check if there are any regressions, and merge this, to be consistent with other operator traits
* run crater, get regressions, write a test for this with a note that "hey, we tried to fix it, its unfixable"
* decide, in the light of by-value DSTs, that this is a feature rather than a bug, and add a test

cc @rust-lang/libs

EDIT: the forth alternative is that there exist a genuine reason why this is the case, but I failed to see it :D
bors added a commit that referenced this pull request Apr 17, 2019
Add test checking that Index<T: ?Sized> works

I've noticed that we have an `Idx: ?Sized` bound on the **index** in the `Index`, which seems strange given that we accept index by value. My guess is that it was meant to be removed in #23601, but was overlooked.

If I remove this bound, `./x.py src/libstd/ src/libcore/` passes, which means at least that this is not covered by test.

I think there's three things we can do here:

* run crater with the bound removed to check if there are any regressions, and merge this, to be consistent with other operator traits
* run crater, get regressions, write a test for this with a note that "hey, we tried to fix it, its unfixable"
* decide, in the light of by-value DSTs, that this is a feature rather than a bug, and add a test

cc @rust-lang/libs

EDIT: the forth alternative is that there exist a genuine reason why this is the case, but I failed to see it :D
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.

4 participants