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.

Sign up for GitHub

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

Jump to bottom

Make Hash, Hasher and BuildHasher #[const_trait] and make Sip const Hasher #104060

Merged
merged 6 commits into from
Nov 10, 2022
Merged

Make Hash, Hasher and BuildHasher #[const_trait] and make Sip const Hasher #104060

merged 6 commits into from
Nov 10, 2022

Conversation

onestacked
Copy link
Contributor

This PR enables using Hashes in const context.

r? @fee1-dead

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2022
@rustbot
Copy link
Collaborator

rustbot commented Nov 6, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 6, 2022
@onestacked onestacked mentioned this pull request Nov 6, 2022
Open
3 tasks
@onestacked
Copy link
Contributor Author

@rustbot label +T-libs-api -T-libs

@rustbot rustbot added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Nov 6, 2022
@onestacked onestacked mentioned this pull request Nov 6, 2022
Merged
@onestacked
Copy link
Contributor Author

@rustbot label +const-hack

@rust-log-analyzer

This comment has been minimized.

Sign in to view
fee1-dead
fee1-dead requested changes Nov 8, 2022
View reviewed changes
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

Left some nits. Also, I would like to see some tests to ensure that it works (maybe not as an ui test, but under library/core/tests, next to the hash tests)

library/core/src/hash/mod.rs Outdated Show resolved Hide resolved
library/core/src/hash/mod.rs Outdated Show resolved Hide resolved
library/core/src/hash/sip.rs Outdated Show resolved Hide resolved
library/std/src/lib.rs Outdated Show resolved Hide resolved
@onestacked
Copy link
Contributor Author

Left some nits. Also, I would like to see some tests to ensure that it works (maybe not as an ui test, but under library/core/tests, next to the hash tests)

Should I add a new file or make the current tests const and add some asserts that check for differences between RT and CT?

@fee1-dead
Copy link
Member

Should I add a new file or make the current tests const and add some asserts that check for differences between RT and CT?

You should add const tests alongside the runtime asserts. Differences between RT and CT are generally not needed to test if you copy all runtime tests and make them const alongside the original tests.

@onestacked
Copy link
Contributor Author

So copy the entire file and make it const there?

@fee1-dead
Copy link
Member

Just the general tests. for example, in https://github.com/rust-lang/rust/blob/master/library/core/tests/hash/mod.rs you don't need test_indirect_hasher and below as those do not currently make sense in const contexts. You could also put the const logic in the same function as the runtime logic using const items.

@onestacked onestacked force-pushed the const_hash branch 2 times, most recently from 8adc0da to d20c739 Compare November 8, 2022 16:35
@fee1-dead
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 10, 2022

📌 Commit 56e59bc has been approved by fee1-dead

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 10, 2022
Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 10, 2022
@Manishearth
Rollup merge of rust-lang#104060 - ink-feather-org:const_hash, r=fee1…
3d848c2 
…-dead

Make `Hash`, `Hasher` and `BuildHasher` `#[const_trait]` and make `Sip` const `Hasher`

This PR enables using Hashes in const context.

r? `@fee1-dead`
@Manishearth Manishearth mentioned this pull request Nov 10, 2022
Closed
@Manishearth Manishearth mentioned this pull request Nov 10, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2022
@bors
Auto merge of rust-lang#104246 - Manishearth:rollup-9o3txc7, r=Manish…
a3c0a02 
…earth

Rollup of 9 pull requests

Successful merges:

 - rust-lang#101939 (Add loongarch64 abi support)
 - rust-lang#103863 (Use `TraitEngine` in more places, restrict visibility of `FulfillmentCtxt` constructor)
 - rust-lang#104036 (Suggest `is_some` when we've found `Option` but expected `bool`)
 - rust-lang#104060 (Make `Hash`, `Hasher` and `BuildHasher` `#[const_trait]` and make `Sip` const `Hasher`)
 - rust-lang#104077 (Use aapcs for efiapi calling convention on arm)
 - rust-lang#104186 (Tighten the 'introduce new binding' suggestion)
 - rust-lang#104194 (`EarlyBinder` docs)
 - rust-lang#104233 (Don't ICE when encountering `ConstKind::Error` in `RequiredConstsVisitor`)
 - rust-lang#104235 (Use `const_error_with_guaranteed` more)

Failed merges:

 - rust-lang#104078 (Print "Checking/Building ..." message even when --dry-run is passed)
 - rust-lang#104169 (Migrate `:target` rules to use CSS variables)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 150e0ec into rust-lang:master Nov 10, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 10, 2022
@onestacked onestacked deleted the const_hash branch February 4, 2023 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fee1-dead fee1-dead fee1-dead approved these changes

Assignees

@fee1-dead fee1-dead

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
5 participants
@onestacked @rustbot @rust-log-analyzer @fee1-dead @bors