-
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] Implement Dict.setdefault(key, default)
#2803
Closed
Closed
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
e6aa1a8
[stdlib] Implement Dict.setdefault()
msaelices 6d3ed56
[stdlib] Tests for Dict.setdefault()
msaelices e81f4de
[stdlib] Update changelog.md
msaelices f1e5bc8
Merge branch 'nightly' into dict-setdefault
msaelices 7b3844f
[stdlib] Use References to speed-up the setdefault() logic
msaelices 1144383
Fix implementation
gabrieldemarmiesse dda4b6d
Merge pull request #1 from gabrieldemarmiesse/show_fix
msaelices a4fd6a1
Merge branch 'nightly' into dict-setdefault
msaelices 7e53b09
[stdlib] Test that checks the number of copies done by setdefault()
msaelices 91d9ee5
[stdlib] Use the transfer operator to make sure there are no copies
msaelices b9f8d4f
Merge branch 'nightly' into dict-setdefault
msaelices f2b4127
[stdlib] Fix the code to work in latest Mojo
msaelices ab36941
[stdlib] Align the return sentence to the except block
msaelices 3450316
Merge branch 'nightly' into dict-setdefault
msaelices 724daf8
Merge branch 'nightly' into dict-setdefault
msaelices eb13caa
Merge branch 'nightly' into dict-setdefault
msaelices 3c989ab
Merge branch 'nightly' into dict-setdefault
msaelices 607b263
Merge branch 'nightly' into dict-setdefault
msaelices 3adb90c
Merge branch 'nightly' into dict-setdefault
ConnorGray File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add another test that verifies that no copies are happening when calling
setdefault
? We can use theCopyCounter
struct for this purpose. There is an example here: https://github.com/modularml/mojo/blob/nightly/stdlib/test/collections/test_list.mojo#L568There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do it when I manage to fix the compiler issue above. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is still this new test to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done here: msaelices@7e53b09
It's curious. I thought the case of the "b" key would have 0 copies but it's 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you should use the transfer operator in the test
^
? Maybe that's why there is a copy. Also I think calling__getitem__
triggers a copyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it but no luck: msaelices@91d9ee5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be a bug in the dict implementation. let's skip this problem for now. I think it needs to be investigated later on, but that would be out of scope for this PR.