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

Stabilize raw ref macros #80886

Merged
merged 1 commit into from
Jan 30, 2021
Merged

Conversation

RalfJung
Copy link
Member

This stabilizes raw_ref_macros (#73394), which is possible now that #74355 is fixed.

However, as I already said in #73394 (comment), I am not particularly happy with the current names of the macros. So I propose we also change them, which means I am proposing to stabilize the following in core::ptr:

pub macro const_addr_of($e:expr) {
    &raw const $e
}

pub macro mut_addr_of($e:expr) {
    &raw mut $e
}

The macro name change means we need another round of FCP. Cc @rust-lang/libs
Fixes #73394

@rust-highfive
Copy link
Collaborator

r? @m-ou-se

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2021
@RalfJung RalfJung added I-nominated T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 10, 2021
@rust-log-analyzer

This comment has been minimized.

@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Jan 10, 2021
@rust-log-analyzer

This comment has been minimized.

@mark-i-m
Copy link
Member

const_raw_ref and mut_raw_ref?

raw_ref and mut_raw_ref?

@RalfJung
Copy link
Member Author

@mark-i-m I don't think "raw references" are a term I've ever seen.

@danielhenrymantilla
Copy link
Contributor

I like the new names in this PR better; although using addr is slightly suboptimal (pointers are not (just) addresses yadda yadda), but besides that pedantic nit I think "addr of" conveys the meaning of C's & / Rust's &raw operator better than speaking of (raw) ref-erences.

Also, tiny nit: could $e be renamed to $place? Since giving a non-place expression yields an error (which is great btw! Having potentially immediately dangling pointers would have been a bit of a footgun, so props on the compilation error).

@tesuji
Copy link
Contributor

tesuji commented Jan 11, 2021

I don't think "raw references" are a term I've ever seen.

Maybe a misunderstand from the feature name: raw_ref_macros ?

@RalfJung
Copy link
Member Author

Also, tiny nit: could $e be renamed to $place? Since giving a non-place expression yields an error

Sure, done.

@danielhenrymantilla
Copy link
Contributor

giving a non-place expression yields an error

Should there be a UI test for this?

@RalfJung
Copy link
Member Author

The error comes from the underlying &raw, I don't think it makes sense to duplicate the test for the macro.
src/test/ui/raw-ref-op/raw-ref-temp.rs tests the underlying &raw.

@mark-i-m
Copy link
Member

@mark-i-m I don't think "raw references" are a term I've ever seen.

@RalfJung Sorry, I'm forgetting what *const T and *mut T are called. Raw pointers? Would const_raw_ptr and mut_raw_ptr be better?

@RalfJung
Copy link
Member Author

Yeah I think "raw pointers" is the usual term, sometimes just "pointers".

The macros are in the ptr namespace, so ptr::const_raw_ptr seems a bit redundant? const_raw on the other hand is hard to decipher, not much better than the current raw_const!. Also, const_raw_ptr!((*x).some_field) is IMO less clear than "address of".

But const_raw_ptr! is definitely better than the current raw_const!. So it would be an okay fallback for me if const_addr_of! doesn't find consensus.

@danielhenrymantilla
Copy link
Contributor

unsafe // Safety: keep it cool :)
fn bikeshed (x: *mut SomeStruct)
{
    use ::core::ptr;

    ptr::raw_const!((*x).some_field);
    ptr::raw_mut!((*x).some_field);

    ptr::const_addr_of!((*x).some_field);
    ptr::mut_addr_of!((*x).some_field);

    // Personal modification of the `{const,mut}_raw_ptr!` suggestion
    ptr::const_ptr_to!((*x).some_field);
    ptr::mut_ptr_to!((*x).some_field);

    // Another personal suggestion
    ptr::raw_ptr!(&raw const (*x).some_field);
    ptr::raw_ptr!(&raw mut (*x).some_field);

    use ::core::ptr::*;

    raw_const!((*x).some_field);
    raw_mut!((*x).some_field);

    const_addr_of!((*x).some_field);
    mut_addr_of!((*x).some_field);

    const_ptr_to!((*x).some_field);
    mut_ptr_to!((*x).some_field);

    raw_ptr!(&raw const (*x).some_field);
    raw_ptr!(&raw mut (*x).some_field);
}
My personal stance (fwiw)

I don't have a strong preference here, except for raw_{const,mut} being the pair that reads the worst. When looking at this, const_addr has the drawback of not necessarily leading to the same muscle memory as const_ptr does, so I think I prefer ptr_to to addr_of for this reason. Finally, the idea of having not two but one macro could be worth exploring 🤷

@RalfJung
Copy link
Member Author

const_ptr_to/mut_ptr_to looks like a reasonable alternative to addr_of.

I don't think we want raw_ptr!(&raw const (*x).some_field); the entire point of this is to not stabilize the &raw syntax.

@mark-i-m
Copy link
Member

@RalfJung We could have it be "generic" though: ptr_to(const (*x).some_field) and ptr_to(mut (*x).some_field)...

@bors

This comment has been minimized.

@m-ou-se
Copy link
Member

m-ou-se commented Jan 14, 2021

Since this is tagged as I-nominated we'll discuss this in the libs meeting next week. But here's some first thoughts:

The names const_addr_of and mut_addr_of make it sound like const and mut apply to the address.

The name const_addr_of makes me think more of const X: T = ..;, const fn and compile time evaluation rather than of the opposite of mut, especially since most const+mut pairs in Rust are called thing+thing_mut instead of thing_const+thing_mut. (I do realise this is specifically not the case for *const T+*mut T, but still.)

Do you have any alternatives for the names in mind? What would you think of something like addr_of and addr_of_mut?

@RalfJung
Copy link
Member Author

RalfJung commented Jan 14, 2021

The name const_addr_of makes me think more of const X: T = ..;, const fn and compile time evaluation rather than of the opposite of mut

Yeah, I know. This is even worse with ptr::raw_const!, which is the current name. The new name at least indicates more clearly that const is an adjective. That's why I think the new names make this confusion less likely, but they certainly do not alleviate it.

especially since most const+mut pairs in Rust are called thing+thing_mut instead of thing_const+thing_mut. (I do realise this is specifically not the case for *const T+*mut T, but still.)

Indeed, we have & and &mut -- but not for raw pointers.

Do you have any alternatives for the names in mind? What would you think of something like addr_of and addr_of_mut?

Some thoughts:

  • I feel it would be strange to deviate from the *const/*mut names of the types that are being constructed here.
  • addr_of_mut sounds like it is taking the address of something mutable, which is IMO the wrong picture -- the right picture is that this takes the address and stores it in a *mut.
  • Mutability in raw pointers is mostly meaningless / "just a lint", but it might still matter sometimes (Coercing &mut to *const should not create a shared reference #56604). So when people want to do mutation I'd still advise against using const_addr_of, so making the const version have the shorter (-> more ergonomic) name (addr_of) is not a good call, in my opinion.

I feel like all choices are bad here, and I had hoped that the libs team would have some good ideas since they certainly have more experience naming Rust APIs than I do. :)

@m-ou-se
Copy link
Member

m-ou-se commented Jan 14, 2021

I feel it would be strange to deviate from the *const/*mut names of the types that are being constructed here.

We already do that for ptr::null+ptr::null_mut and ptr::slice_from_raw_parts+ptr::slice_from_raw_parts_mut though.

addr_of_mut sounds like it is taking the address of something mutable, which is IMO the wrong picture

Even though const/mut in pointers is mostly meaningless, isn't *mut T is still supposed to point to a mutable T?

the right picture is that this takes the address and stores it in a *mut.

How would you pronounce *mut T? "mut pointer"? "pointer to mut"? Or something else?

@RalfJung
Copy link
Member Author

Even though const/mut in pointers is mostly meaningless, isn't *mut T is still supposed to point to a mutable T?

I guess it is, but personally on the few occasions that I have been writing raw pointer code, that's not how I thought about this. But maybe I am thinking about this in weird ways.

How would you pronounce *mut T? "mut pointer"? "pointer to mut"? Or something else?

I'd say "mutable raw pointer", and "const(ant) raw pointer" for *const T.

@RalfJung RalfJung added the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Jan 23, 2021
@RalfJung
Copy link
Member Author

RalfJung commented Jan 29, 2021

All right, I renamed the macros accordingly. I left the version at 1.51, hoping this will land until the next beta branches on Monday. :D

EDIT: Oh, it's Monday the week after the next. No rush then.^^

@m-ou-se
Copy link
Member

m-ou-se commented Jan 29, 2021

FCP with the old names (raw_const, raw_mut) happened here: #73394 (comment).

The new names are the result of a meeting which was attended by five libs team members.

So, no need for a new FCP, as the names are just a small change.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 29, 2021

📌 Commit 13ffa43 has been approved by m-ou-se

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 30, 2021
Rollup of 16 pull requests

Successful merges:

 - rust-lang#79023 (Add `core::stream::Stream`)
 - rust-lang#80562 (Consider Scalar to be a bool only if its unsigned)
 - rust-lang#80886 (Stabilize raw ref macros)
 - rust-lang#80959 (Stabilize `unsigned_abs`)
 - rust-lang#81291 (Support FRU pattern with `[feature(capture_disjoint_fields)]`)
 - rust-lang#81409 (Slight simplification of chars().count())
 - rust-lang#81468 (cfg(version): treat nightlies as complete)
 - rust-lang#81473 (Warn write-only fields)
 - rust-lang#81495 (rustdoc: Remove unnecessary optional)
 - rust-lang#81499 (Updated Vec::splice documentation)
 - rust-lang#81501 (update rustfmt to v1.4.34)
 - rust-lang#81505 (`fn cold_path` doesn't need to be pub)
 - rust-lang#81512 (Add missing variants in match binding)
 - rust-lang#81515 (Fix typo in pat.rs)
 - rust-lang#81519 (Don't print error output from rustup when detecting default build triple)
 - rust-lang#81520 (Don't clone LLVM submodule when download-ci-llvm is set)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b94d84d into rust-lang:master Jan 30, 2021
@rustbot rustbot added this to the 1.51.0 milestone Jan 30, 2021
JohnTitor added a commit to JohnTitor/miri that referenced this pull request Jan 30, 2021
JohnTitor added a commit to JohnTitor/miri that referenced this pull request Jan 30, 2021
@RalfJung RalfJung deleted the stable-raw-ref-macros branch January 31, 2021 12:44
cuviper pushed a commit to cuviper/rust that referenced this pull request Feb 12, 2021
yvt added a commit to r3-os/r3 that referenced this pull request Feb 19, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 28, 2021
…=m-ou-se

Stabilize core::task::ready!

_Tracking issue: https://github.com/rust-lang/rust/issues/70922_

This PR stabilizes the `task::ready!` macro. Similar to rust-lang#80886, this PR was waiting on rust-lang#74355 to be fixed.

The `task::ready!` API has existed in the futures ecosystem for several years, and was added on nightly last year in rust-lang#70817. The motivation for this macro is the same as it was back then: virtually every single manual future implementation makes use of this; so much so that it's one of the few things included in the [futures-core](https://docs.rs/futures-core/0.3.12/futures_core) library.

r? `@tmandry`

cc/ `@rust-lang/wg-async-foundations` `@rust-lang/libs`

## Example
```rust
use core::task::{Context, Poll};
use core::future::Future;
use core::pin::Pin;

async fn get_num() -> usize {
    42
}

pub fn do_poll(cx: &mut Context<'_>) -> Poll<()> {
    let mut f = get_num();
    let f = unsafe { Pin::new_unchecked(&mut f) };

    let num = ready!(f.poll(cx));
    // ... use num

    Poll::Ready(())
}
```
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 28, 2021
…=m-ou-se

Stabilize core::task::ready!

_Tracking issue: https://github.com/rust-lang/rust/issues/70922_

This PR stabilizes the `task::ready!` macro. Similar to rust-lang#80886, this PR was waiting on rust-lang#74355 to be fixed.

The `task::ready!` API has existed in the futures ecosystem for several years, and was added on nightly last year in rust-lang#70817. The motivation for this macro is the same as it was back then: virtually every single manual future implementation makes use of this; so much so that it's one of the few things included in the [futures-core](https://docs.rs/futures-core/0.3.12/futures_core) library.

r? ``@tmandry``

cc/ ``@rust-lang/wg-async-foundations`` ``@rust-lang/libs``

## Example
```rust
use core::task::{Context, Poll};
use core::future::Future;
use core::pin::Pin;

async fn get_num() -> usize {
    42
}

pub fn do_poll(cx: &mut Context<'_>) -> Poll<()> {
    let mut f = get_num();
    let f = unsafe { Pin::new_unchecked(&mut f) };

    let num = ready!(f.poll(cx));
    // ... use num

    Poll::Ready(())
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. 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
Development

Successfully merging this pull request may close these issues.

Tracking Issue for raw_ref_macros