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

Replace const fn name() with associated NAME constants #715

Merged
merged 1 commit into from
Mar 9, 2023

Conversation

MarijnS95
Copy link
Collaborator

@MarijnS95 MarijnS95 commented Mar 9, 2023

Depends on #716

CStr::from_bytes_with_nul_unchecked is const-stable since Rust 1.59 which is already required for ash so it is high time to finally turn these inlined name() functions into associated constants (which is a breaking change in itself that cannot be backported).

@MarijnS95 MarijnS95 force-pushed the fn-name-to-assoc-const branch 4 times, most recently from 5102303 to e288193 Compare March 9, 2023 21:26
Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

🎉

Any compiletime impact? Not sure which way I'd expect this to go.

@MarijnS95
Copy link
Collaborator Author

MarijnS95 commented Mar 9, 2023

e288193 (this PR):

$ hyperfine -p 'cargo clean -p ash' 'cargo b -p ash'
Benchmark 1: cargo b -p ash
  Time (mean ± σ):      6.105 s ±  0.057 s    [User: 6.905 s, System: 0.529 s]
  Range (min … max):    6.046 s …  6.228 s    10 runs

197bc30 (HEAD~, i.e. #716):

$ hyperfine -p 'cargo clean -p ash' 'cargo b -p ash'
Benchmark 1: cargo b -p ash
  Time (mean ± σ):      6.126 s ±  0.107 s    [User: 6.916 s, System: 0.517 s]
  Range (min … max):    5.994 s …  6.373 s    10 runs

That's identical.

`CStr::from_bytes_with_nul_unchecked` is `const`-stable since Rust 1.59
which is already required for `ash` so it is high time to finally turn
these inlined `name()` functions into associated constants (which is a
breaking change in itself that cannot be backported).
@MarijnS95 MarijnS95 merged commit 7a16860 into master Mar 9, 2023
@MarijnS95 MarijnS95 deleted the fn-name-to-assoc-const branch March 9, 2023 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants