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 zty0826@gmail.com

@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.

Signed-off-by: TennyZhuang <zty0826@gmail.com>
Signed-off-by: TennyZhuang <zty0826@gmail.com>
@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).

Signed-off-by: TennyZhuang <zty0826@gmail.com>
@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 <zty0826@gmail.com>
@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
Collaborator

bors commented Oct 4, 2022

📌 Commit 92618d7 has been approved by Amanieu

It is now in the queue for this repository.

@bors
Copy link
Collaborator

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 <zty0826@gmail.com>
@bors
Copy link
Collaborator

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
Collaborator

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 <zty0826@gmail.com>
@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
Collaborator

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
Collaborator

bors commented Oct 8, 2022

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

@bors
Copy link
Collaborator

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.

None yet

4 participants