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

make with_hasher_in const (consistent with with_hasher) #355

Merged
merged 4 commits into from
Oct 8, 2022

Conversation

TennyZhuang
Copy link
Contributor

@TennyZhuang TennyZhuang commented Aug 22, 2022

I found that the following four methods have additional comments about their allocation:

  • HashMap::with_hasher
  • HashMap::with_hasher_in
  • HashSet::with_hasher
  • HashSet::with_hasher_in

There is no reason to make such a difference; in this PR, I make them consistent as the following behavior:

  • All methods clarify that they will not allocate memory before the first insertion
  • All methods are marked as const function.

We also bump the MSRV to 1.61.0 for the const_fn_trait_bound feature. Currently, 1.64.0 has been released, and I guess it's acceptable to break the 1.61.0 users.

Signed-off-by: TennyZhuang [email protected]

@TennyZhuang
Copy link
Contributor Author

rust-lang/rust#93706, seems that it depends a feature which was stabilized?

@Amanieu
Copy link
Member

Amanieu commented Aug 31, 2022

You need to bump the minimum Rust version in CI to 1.61 which is when the const_fn_trait_bound feature was stabilized.

@TennyZhuang
Copy link
Contributor Author

You need to bump the minimum Rust version in CI to 1.61 which is when the const_fn_trait_bound feature was stabilized.

I prefer to do that after 1.64 stabilized, just hold the PR.

@TennyZhuang TennyZhuang force-pushed the make-with-hasher-in-const branch from d8a2ed1 to bdf3e2d Compare October 2, 2022 01:40
@TennyZhuang
Copy link
Contributor Author

Why were most actions canceled without errors? Are there some limitations?

@Amanieu
Copy link
Member

Amanieu commented Oct 2, 2022

The runs are canceled once the first runner fails. In this case it seems that the 1.56.1 runner is failing, because you missed that one when changing the workflows (line 69).

@TennyZhuang TennyZhuang force-pushed the make-with-hasher-in-const branch from bdf3e2d to 630f3a7 Compare October 2, 2022 16:16
@TennyZhuang
Copy link
Contributor Author

TennyZhuang commented Oct 2, 2022

The runs are canceled once the first runner fails. In this case it seems that the 1.56.1 runner is failing, because you missed that one when changing the workflows (line 69).

Oh, I'm curious why I miss it because I find & replace it globally. I only replaced 1.58.1 :( Thank you very much!

Signed-off-by: TennyZhuang <[email protected]>
@Amanieu
Copy link
Member

Amanieu commented Oct 3, 2022

Thanks!

@Amanieu
Copy link
Member

Amanieu commented Oct 3, 2022

bors r+

@Amanieu
Copy link
Member

Amanieu commented Oct 4, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 4, 2022

📌 Commit 92618d7 has been approved by Amanieu

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 4, 2022

⌛ Testing commit 92618d7 with merge d59f64e...

bors added a commit that referenced this pull request Oct 4, 2022
make with_hasher_in const (consistent with with_hasher)

I found that the following four methods have additional comments about their allocation:

* HashMap::with_hasher
* HashMap::with_hasher_in
* HashSet::with_hasher
* HashSet::with_hasher_in

There is no reason to make such a difference; in this PR, I make them consistent as the following behavior:

* All methods clarify that they will not allocate memory before the first insertion
* All methods are marked as const function.

We also bump the MSRV to 1.61.0 for the const_fn_trait_bound feature. Currently, 1.64.0 has been released, and I guess it's acceptable to break the 1.61.0 users.

Signed-off-by: TennyZhuang <[email protected]>
@bors
Copy link
Contributor

bors commented Oct 4, 2022

💔 Test failed - checks-actions

@Amanieu
Copy link
Member

Amanieu commented Oct 6, 2022

I can't reproduce this failure locally, let's try again?
@bors retry

@bors
Copy link
Contributor

bors commented Oct 6, 2022

⌛ Testing commit 92618d7 with merge e674100...

bors added a commit that referenced this pull request Oct 6, 2022
make with_hasher_in const (consistent with with_hasher)

I found that the following four methods have additional comments about their allocation:

* HashMap::with_hasher
* HashMap::with_hasher_in
* HashSet::with_hasher
* HashSet::with_hasher_in

There is no reason to make such a difference; in this PR, I make them consistent as the following behavior:

* All methods clarify that they will not allocate memory before the first insertion
* All methods are marked as const function.

We also bump the MSRV to 1.61.0 for the const_fn_trait_bound feature. Currently, 1.64.0 has been released, and I guess it's acceptable to break the 1.61.0 users.

Signed-off-by: TennyZhuang <[email protected]>
@Amanieu
Copy link
Member

Amanieu commented Oct 6, 2022

Actually this seems to be caused by: rust-lang/rust#102748

Let's wait for that fix to get merged first.

@bors
Copy link
Contributor

bors commented Oct 6, 2022

💔 Test failed - checks-actions

@JustForFun88
Copy link
Contributor

JustForFun88 commented Oct 8, 2022

I think it can be merged now, but please don't bump the version of hashbrown crate because I want to document a raw part of crate

@Amanieu
Copy link
Member

Amanieu commented Oct 8, 2022

@bors retry

@bors
Copy link
Contributor

bors commented Oct 8, 2022

⌛ Testing commit 92618d7 with merge 5d9988c...

@bors
Copy link
Contributor

bors commented Oct 8, 2022

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 5d9988c to master...

@bors bors merged commit 5d9988c into rust-lang:master Oct 8, 2022
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