Skip to content

likely/unlikely intrinsics don't need unsafe #45445

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

Closed
gnzlbg opened this issue Oct 22, 2017 · 8 comments
Closed

likely/unlikely intrinsics don't need unsafe #45445

gnzlbg opened this issue Oct 22, 2017 · 8 comments
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Oct 22, 2017

Minor paper-cut, but these intrinsics are safe.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Oct 22, 2017

Relates to #45440. If we can get them to propagate properly through inline(always) functions, we would be able to write a "safe" wrapper around them that we can stabilize.

@hanna-kruppe
Copy link
Contributor

These are far from the only intrinsics that are safe, but there's currently no way to make intrinsics safe (short of a safe wrapper). I think I saw an issue about this at some point but I can't find it now.

@est31
Copy link
Member

est31 commented Oct 22, 2017

For safe wrappers for likely/unlikely intrinsics, one can also do macros with allow_internal_unsafe: see it live:

#[allow_internal_unsafe]
macro_rules! unlikely {
  ($e:expr) => {
    unsafe {
      unlikely($e)
    }
  };
}

You can pair it with allow_internal_unstable to expose stable unlikely/likely macros!

@TimNN TimNN added C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Oct 22, 2017
@jbayardo
Copy link

jbayardo commented Oct 23, 2017

The snippet by @est31 should be:

#[allow_internal_unsafe]
macro_rules! unlikely {
  ($e:expr) => {
    #[allow(unused_unsafe)]
    unsafe {
      std::intrinsics::unlikely($e)
    }
  };
}

And requires

#![feature(core_intrinsics)]
#![feature(allow_internal_unsafe)]
#![feature(stmt_expr_attributes)]

In order to work seamlessly as of today's nightly (in the case of likely/unlikely used within unsafe blocks).

@est31
Copy link
Member

est31 commented Oct 23, 2017

@jbayardo AFAIK you can also attach the #[allow(unused_unsafe)] to the macro declaration, it should work too. The entire macro can theoretically be exposed by the standard library.

@jbayardo
Copy link

That did not work for me. I believe this shouldn't be a macro though, the likely/unlikely intrinsics should just be safe; the #allow(unused_unsafe) won't warn you in cases like

unsafe {
  ...
  if likely!(unsafe { ... }) {
    ...
  } else {
    ...
  }
}

@hanna-kruppe
Copy link
Contributor

You can't just mark intrinsics safe, because all intrinsics are extern "rust-intrinsic" functions and extern functions are always unsafe to call. Changing that would require some new language feature, e.g., as proposed by #1248.

@dtolnay
Copy link
Member

dtolnay commented Nov 14, 2017

As mentioned, there are many other intrinsics that should not require unsafe. This is going to need an RFC around how to expose intrinsics that are safe to call. One previous RFC on this topic was closed rust-lang/rfcs#1248 (comment) but the lang team plans to follow up by merging the idea of "intrinsic" and "lang item", at which point likely and unlikely would be exposed as safe to call.

@dtolnay dtolnay closed this as completed Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: A feature request, i.e: not implemented / a PR. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants