-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[stdlib] List __getitem__ returns auto-dereferenced ref #2847
Conversation
f8f9a0f
to
bdf1abd
Compare
!sync |
Unfortunately this is hitting some compiler crashes internally. I'll file a GitHub issue, but until it's solved, I don't see an easy workaround for this really. |
fe95cea
to
3db296c
Compare
3db296c
to
b0dd56b
Compare
!sync |
b0dd56b
to
dea683a
Compare
Signed-off-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com>
Signed-off-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com>
Signed-off-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com>
dea683a
to
c17b494
Compare
Hey @mikowals I just wanted to share an update on this PR. @lattner personally had a look at the bug holding this up🙂 He mentioned the error message we were seeing could use some improvement, but realized that the issue was an ambiguity between whether the compiler should call He was able to fix this by creating a patch that updated The other callsites updated and tests you added in this PR are still awesome to have, so I'm going rebase this PR and merge those changes in :) Thank you again for pushing on this improvement and your patience while it was investigated and fixed 🙂 If you're interested in continuing work in this vein ⛏️ , there's still a lot of low-hanging fruit in terms of places in the stdlib that need to be updated to use reference—we'd love to work with you on any future contributions that interest you🙂 |
!sync |
✅🟣 This contribution has been merged 🟣✅ Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours. We use Copybara to merge external contributions, click here to learn more. |
…40663) [External] [stdlib] Add tests for `List.__getitem__` returns auto-dereferenced ref `List.__getitem__()` no longer makes copies when returning a value. I also added a test to show that setting an individual field using sugared `my_list[0].value = 1` no longer produces extra copies. Passing that test is why `__getitem__` receives a `Reference[Self, ...]` argument rather than just `self`. I removed uses of `__get_ref` in `list.mojo` and `test_list.mojo` other than leaving one test to show that `__get_ref` was still functioning correctly until it is removed. I left usage of `List.__get_ref` in other modules untouched. From a few quick efforts to eliminate `__get_ref` it was leading to some lifetime casting decisions that seemed complex and numerous enough they should be handled separately. ORIGINAL_AUTHOR=Michael Kowalski <1331470+mikowals@users.noreply.github.com> PUBLIC_PR_LINK=#2847 Co-authored-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com> Closes #2847 MODULAR_ORIG_COMMIT_REV_ID: 3a03e1669aad93bd512210b373c05df0e2adb313
Landed in 7c00fc9! Thank you for your contribution 🎉 |
Amazing, thank you @mikowals !! |
…40663) [External] [stdlib] Add tests for `List.__getitem__` returns auto-dereferenced ref `List.__getitem__()` no longer makes copies when returning a value. I also added a test to show that setting an individual field using sugared `my_list[0].value = 1` no longer produces extra copies. Passing that test is why `__getitem__` receives a `Reference[Self, ...]` argument rather than just `self`. I removed uses of `__get_ref` in `list.mojo` and `test_list.mojo` other than leaving one test to show that `__get_ref` was still functioning correctly until it is removed. I left usage of `List.__get_ref` in other modules untouched. From a few quick efforts to eliminate `__get_ref` it was leading to some lifetime casting decisions that seemed complex and numerous enough they should be handled separately. ORIGINAL_AUTHOR=Michael Kowalski <1331470+mikowals@users.noreply.github.com> PUBLIC_PR_LINK=#2847 Co-authored-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com> Closes #2847 MODULAR_ORIG_COMMIT_REV_ID: 3a03e1669aad93bd512210b373c05df0e2adb313
With this,
List.__getitem__()
no longer makes copies when returning a value. I also added a test to show that setting an individual field using sugaredmy_list[0].value = 1
no longer produces extra copies. Passing that test is why__getitem__
receives aReference[Self, ...]
argument rather than justself
.I removed uses of
__get_ref
inlist.mojo
andtest_list.mojo
other than leaving one test to show that__get_ref
was still functioning correctly until it is removed. I left usage ofList.__get_ref
in other modules untouched. From a few quick efforts to eliminate__get_ref
it was leading to some lifetime casting decisions that seemed complex and numerous enough they should be handled separately.This supersedes #2544 which I will close. It should close #2432 and #2540 as
__refitem__
and it related difficulties are no longer relevant.