Simplify likely! and unlikely! macro
#97895
Conversation
rustbot
added
the
T-compiler
|
r? @estebank (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
Are these macros even needed then? |
|
Personally I feel use a macro is better than direct use of intrinsics (it's easier to replace the implementation to a no-op if intrinsics use is to be avoided). With that said, an re-export could serve the same purpose. Or maybe we don't even need to worry about that given that the code in the compiler itself so the intrinsics won't go away. |
|
Funnily enough, I didn't find any uses of Anyone against removing the macros, @rust-lang/compiler? |
|
I prefer the macros. They read different than the intrinsics. |
|
|
|
I would prefer to use the functions directly if the macros don't add any new behavior 🤷 |
|
C++20 went for attributes. The if or else branch are likely and not the value. |
|
Using the intrinsic directly seems preferable to me. I suspect the macros were mostly for convenience when this still required unsafe. |
|
@nbdd0121 would you mind removing the macros and using the intrinsic directly? I think that'll be better given the context we have at the moment. |
|
Re-export or direct use? |
|
Re-export is fine, but direct use might be better. |
|
Done. The |
| @@ -804,7 +804,7 @@ pub type PolyTraitPredicate<'tcx> = ty::Binder<'tcx, TraitPredicate<'tcx>>; | |||
|
|
|||
| impl<'tcx> TraitPredicate<'tcx> { | |||
| pub fn remap_constness(&mut self, tcx: TyCtxt<'tcx>, param_env: &mut ParamEnv<'tcx>) { | |||
| if unlikely!(Some(self.trait_ref.def_id) == tcx.lang_items().drop_trait()) { | |||
| if std::intrinsics::unlikely(Some(self.trait_ref.def_id) == tcx.lang_items().drop_trait()) { | |||
There was a problem hiding this comment.
should we remove this now?
cc @fee1-dead
There was a problem hiding this comment.
Yeah, this should be removed.
There was a problem hiding this comment.
Just to be clear, this isn't something for me to action on, right?
There was a problem hiding this comment.
No, I have added it to my to-do list. Although if you would allow me, I can just edit this PR to include the removal of this chunk :)
There was a problem hiding this comment.
I'd say probably it's better to have this PR merged first (cuz the changes are not really correlated so doesn't really fit into a single PR in my mind). @estebank are there anything blocking you from r+ing this PR?
|
@bors r+ |
|
📌 Commit 8b7299d has been approved by |
bors
added
S-waiting-on-bors
Simplify `likely!` and `unlikely!` macro The corresponding intrinsics have long been safe-to-call, so the unsafe block is no longer needed.
Rollup of 10 pull requests Successful merges: - rust-lang#95446 (update CPU usage script) - rust-lang#96768 (Use futex based thread parker on Fuchsia.) - rust-lang#97454 (Add release notes for 1.62) - rust-lang#97516 (clarify how Rust atomics correspond to C++ atomics) - rust-lang#97818 (Point at return expression for RPIT-related error) - rust-lang#97895 (Simplify `likely!` and `unlikely!` macro) - rust-lang#98005 (Add some tests for impossible bounds) - rust-lang#98226 (Document unstable `--extern` options) - rust-lang#98356 (Add missing period) - rust-lang#98363 (remove use of &Alloc in btree tests) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
The corresponding intrinsics have long been safe-to-call, so the unsafe block is no longer needed.