Skip to content

/rust

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

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@nikomatsakis @Gankro @japaric @bors
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Mar 22, 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
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 22, 2015

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

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:by-value-index branch from 5ca7257 to bae844d Mar 22, 2015

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 22, 2015

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 22, 2015

oh and cc @sfackler
@Gankro

This comment has been minimized.

Sign in to view
Copy link
Contributor

Gankro 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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 22, 2015

ok, make check passes.
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 22, 2015

@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

This comment has been minimized.

Sign in to view
Copy link
Member

japaric commented Mar 22, 2015

@bors: r+ bae844d
@bors

This comment has been minimized.

Sign in to view
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 nikomatsakis force-pushed the nikomatsakis:by-value-index branch from bae844d to b760c56 Mar 23, 2015

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 23, 2015

@bors r=japaric b760c56
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 23, 2015

@bors p=1

important language change
@bors

This comment has been minimized.

Sign in to view
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

@bors
Auto merge of #23601 - nikomatsakis:by-value-index, r=japaric 
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
2aa6858
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 23, 2015

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 23, 2015

@bors: retry

this test failure makes no sense!
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 23, 2015

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

nikomatsakis added some commits Mar 21, 2015

@nikomatsakis
Adjust Index/IndexMut impls. For generic collections, we take 
references. For collections whose keys are integers, we take both
references and by-value.
b4d4daf
@nikomatsakis
Fallout in stdlib, rustdoc, rustc, etc. For most maps, converted uses of 
`[]` on maps to `get` in rustc, since stage0 and stage1+ disagree about
how to use `[]`.
8e58af4
@nikomatsakis
Update borrowck tests to test that index is by-move now
Loading status checks…
57cf2de

@nikomatsakis nikomatsakis force-pushed the nikomatsakis:by-value-index branch from b760c56 to 57cf2de Mar 23, 2015

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 23, 2015

@bors r=japaric 57cf2de p=1

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Mar 23, 2015

@alexcrichton
rollup merge of rust-lang#23601: nikomatsakis/by-value-index 
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
753efb5
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⌛️ Testing commit 57cf2de with merge beb33d4...

bors added a commit that referenced this pull request Mar 24, 2015

@bors
Auto merge of #23601 - nikomatsakis:by-value-index, r=japaric 
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
beb33d4
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⛄️ The build was interrupted to prioritize another pull request.
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⌛️ Testing commit 57cf2de with merge ffd5f00...

bors added a commit that referenced this pull request Mar 24, 2015

@bors
Auto merge of #23601 - nikomatsakis:by-value-index, r=japaric 
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
ffd5f00
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

⛄️ The build was interrupted to prioritize another pull request.
@bors

This comment has been minimized.

Sign in to view
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

@bors
Auto merge of #23601 - nikomatsakis:by-value-index, r=japaric 
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
de8a23b
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Mar 24, 2015

💔 Test failed - auto-linux-64-opt
@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

nikomatsakis commented Mar 24, 2015

@bors: retry

@bors bors merged commit 57cf2de into rust-lang:master Mar 24, 2015
1 of 2 checks passed

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@alexcrichton alexcrichton referenced this pull request Mar 26, 2015

Closed

Indexing Autoref + Borrow = :( #22826

@nikomatsakis nikomatsakis deleted the nikomatsakis:by-value-index branch Mar 30, 2016

@matklad matklad referenced this pull request Mar 29, 2019

Open

remove ?Sized bounds from Index #59527

bors added a commit that referenced this pull request Mar 29, 2019

@bors
Auto merge of #59527 - matklad:sized-index, r=<try> 
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
Loading status checks…
47f4f94
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.