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

[stdlib] Fix dict probing error #2351

Closed
wants to merge 4 commits into from

Conversation

mzaks
Copy link
Contributor

@mzaks mzaks commented Apr 20, 2024

Fixes #1729

@mzaks mzaks requested a review from a team as a code owner April 20, 2024 08:57
Copy link
Contributor

@bethebunny bethebunny left a comment

Choose a reason for hiding this comment

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

Small style changes, looks great, thank you so much!

Comment on lines 755 to 781
var insert_slot = Optional[Int]()
var insert_index = Optional[Int]()
Copy link
Contributor

Choose a reason for hiding this comment

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

We can get rid of these definitions too

Comment on lines 291 to 308
def test_probing_error():
var keys = List(
7005684093727295727,
2833576045803927472,
-446534169874157203,
-5597438459201014662,
-7007119737006385570,
7237741981002255125,
-649171104678427962,
-6981562940350531355,
)
var d = Dict[DummyKey, Int]()
for i in range(len(keys)):
d[DummyKey(keys[i])] = i
assert_equal(len(d), len(keys))
for i in range(len(d)):
var k = keys[i]
assert_equal(i, d[k])
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

return self.value != other.value


def test_probing_error():
Copy link
Contributor

Choose a reason for hiding this comment

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

can you rename this to test_mojo_issue_1729? that'll help point folks towards the issue context if/when someone trips this later

Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
@ConnorGray
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Hi @mzaks thank you fixing this bug, great catch!

We're moving to a new infrastructure for merging contributions to Mojo (we're using a tool called Copybara), and your contribution has now been merged into our internal copy of the Mojo Standard Library.

The changes in this PR will appear here in the mojo repo nightly branch when we do our next outbound synchronization at the time that the next Mojo nightly is released. That should happen later today but could be delayed until Monday.

Thank you so much for the amazing catch 🔍 finding this bug and your 🔥 contribution fixing it :)

Please let me know if you have any questions or concerns.

@ConnorGray ConnorGray closed this Apr 26, 2024
patrickdoc pushed a commit that referenced this pull request Apr 26, 2024
[External] [stdlib] Fix dict probing error

Fixes #1729

Co-authored-by: Maxim Zaks <maxim.zaks@gmail.com>
Closes #2351
MODULAR_ORIG_COMMIT_REV_ID: d48102f8ac6ad9cfdd5df57c344ce41d7b5cff4c
jayzhan211 pushed a commit to jayzhan211/mojo that referenced this pull request Apr 27, 2024
[External] [stdlib] Fix dict probing error

Fixes modularml#1729

Co-authored-by: Maxim Zaks <maxim.zaks@gmail.com>
Closes modularml#2351
MODULAR_ORIG_COMMIT_REV_ID: d48102f8ac6ad9cfdd5df57c344ce41d7b5cff4c
patrickdoc pushed a commit that referenced this pull request May 2, 2024
[External] [stdlib] Fix dict probing error

Fixes #1729

Co-authored-by: Maxim Zaks <maxim.zaks@gmail.com>
Closes #2351
MODULAR_ORIG_COMMIT_REV_ID: d48102f8ac6ad9cfdd5df57c344ce41d7b5cff4c
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.

None yet

3 participants