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

Reexport std::hint::black_box #701

Closed
wants to merge 7 commits into from
Closed

Conversation

Spartan2909
Copy link

Fixes #700.

@waywardmonkeys
Copy link
Contributor

Would it make sense to possibly have a black_box function still, but mark it as deprecated and suggest calling std::hint::black_box instead, so that people can migrate?

@Spartan2909
Copy link
Author

That's also a good option. @bheisler any opinions?

@lemmih
Copy link
Collaborator

lemmih commented Jul 19, 2023

That's also a good option. @bheisler any opinions?

I think that sounds like a good idea.

@bheisler
Copy link
Owner

Oh, wow. They finally stabilized std::hint::black_box? I didn't think that would ever happen.

Yeah, deprecating our hacky replacement for black_box seems good to me, but keep in mind the minimum-supported-Rust-version policy. It wouldn't seem right to deprecate in favor of a stdlib function that hasn't existed for at least three major versions.

@waywardmonkeys
Copy link
Contributor

Stable (const unstable) since 1.66 ... so that's 5 versions now if I count correctly. :)

@Spartan2909
Copy link
Author

I've added a deprecated wrapper for std::hint::black_box.

@waywardmonkeys
Copy link
Contributor

You will probably need to bump the MSRV in CI and elsewhere since it is currently stating and testing 1.64

@waywardmonkeys
Copy link
Contributor

Alternatively, deprecate in favor of std::hint but keep the old implementation for those that don’t want to update? Up to the maintainers!

@d-e-s-o
Copy link
Contributor

d-e-s-o commented Jul 21, 2023

Any chance the real_blackbox feature could be cleaned up in the process? Or is this a compat hazard?

@Spartan2909
Copy link
Author

Spartan2909 commented Jul 22, 2023

I've bumped the MSRV and removed the use of the test feature. @d-e-s-o removing a feature is a
breaking change, but could be done with a minor version bump as Criterion is still <1.0. I'll leave this up to the maintainers.

@lilyball
Copy link

I would really love to see this merged and a new release published. asan keeps sporadically triggering a false positive on the current black_box impl when used with zero-sized types so this change is more than just a performance fix.

@Spartan2909
Copy link
Author

@bheisler Is there anything blocking this?

waywardmonkeys added a commit to waywardmonkeys/criterion.rs that referenced this pull request Jul 25, 2024
This has been stable since Rust 1.66 and our MSRV is now 1.70.

The feature is kept around for now as removing it would break
people's configurations if they were enabling it.

It can be removed at the next semver break.

Supercedes bheisler#701.
Fixes bheisler#633.
Fixes bheisler#700.
lemmih pushed a commit that referenced this pull request Jul 26, 2024
This has been stable since Rust 1.66 and our MSRV is now 1.70.

The feature is kept around for now as removing it would break
people's configurations if they were enabling it.

It can be removed at the next semver break.

Supercedes #701.
Fixes #633.
Fixes #700.
@waywardmonkeys
Copy link
Contributor

@lemmih This can be closed now that #794 has landed. Thanks to @Spartan2909 for the work done here as helped pave the way!

@lemmih lemmih closed this Jul 27, 2024
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.

The black_box function can be replaced with std::hint::black_box
6 participants