-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Explain fully qualified syntax for Rc
and Arc
#76138
Conversation
r? @cramertj (rust_highfive has picked a reviewer for you, use r? to override) |
@@ -35,15 +35,26 @@ | |||
//! `Rc<T>` automatically dereferences to `T` (via the [`Deref`] trait), | |||
//! so you can call `T`'s methods on a value of type [`Rc<T>`][`Rc`]. To avoid name | |||
//! clashes with `T`'s methods, the methods of [`Rc<T>`][`Rc`] itself are associated | |||
//! functions, called using function-like syntax: | |||
//! functions, called using [fully qualified syntax]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually call this "fully qualified syntax"? I thought that would be reserved for <RC>::downgrade
form.
Naively, "function like syntax" or "associated function syntax" seem like better options to me here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, "fully-qualified sytax" is used for the form with the <>
s. I would drop the extra clause, and just end after "functions."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, wouldn’t it be better to give the readers somewhere to read more about fully qualified syntax? Side note, shouldn’t it be “fully-qualified syntax” with a hyphen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn’t it be better to give the readers somewhere to read more about fully qualified syntax?
Maybe. I am purely thinking about the wording itself.
Side note, shouldn’t it be “fully-qualified syntax” with a hyphen?
I don't think so; the hyphen would be used if it is confusing which part "qualified" is attached to, that is, to disambiguate between
fully-qualified syntax
fully qualified-syntax
but I don't think that the latter would be a common interpretation
library/alloc/src/rc.rs
Outdated
//! `Rc<T>`'s implementations of traits like `Clone` should also be called using | ||
//! fully qualified syntax to avoid confusion as to whether the *reference* is being | ||
//! cloned or the *backing data* (`T`) is being cloned: | ||
//! | ||
//! ``` | ||
//! use std::rc::Rc; | ||
//! | ||
//! let my_rc = Rc::new(()); | ||
//! let your_rc = Rc::clone(&my_rc); | ||
//! ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that we explicitly decided against this in #63252.
My personal opinion is that people do use Arc::clone
syntax in the wild, so this should be in the official docs one way or another, but probably without "should" or "idiomatic" langauge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urgh, but #63252 didn't update
I guess, the status quo is confused here, and we should probably try to be consistent at least withing stdlib itself :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We teach the ::clone
version in the book too; I wasn't notified of that thread either :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned the ::clone
version (I guess from the book?) and I think it has clearer intent. Maybe for now we’ll go with it here and then we can change it later if we end up updating places like in the book?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for not using ::clone
as per #63252
It's no longer the official guidance, and I don't think the existence of its use in other resources is good justification for continuing to use it in example code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still waiting for this change, @camelid . (I don't like it but it appears to be the consensus.)
☔ The latest upstream changes (presumably #76235) made this pull request unmergeable. Please resolve the merge conflicts. |
66444e3
to
9f59bc6
Compare
Rebased. |
☔ The latest upstream changes (presumably #73996) made this pull request unmergeable. Please resolve the merge conflicts. |
Not again! |
9f59bc6
to
3cdd7cb
Compare
Rebased. |
@@ -1266,7 +1266,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o { | |||
} else { | |||
err.span_suggestion( | |||
span, | |||
"use fully-qualified syntax", | |||
"use fully qualified syntax", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was the hyphen here removed? It seems correct to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree; I think it's correct with the hyphen. The issue is that it's referred to without the hyphen everywhere else in the compiler and in the book. I figure that unless and until we change it elsewhere, it should be consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See @steveklabnik
's comment: #76138 (comment).
library/alloc/src/rc.rs
Outdated
//! `Rc<T>`'s implementations of traits like `Clone` should also be called using | ||
//! fully qualified syntax to avoid confusion as to whether the *reference* is being | ||
//! cloned or the *backing data* (`T`) is being cloned: | ||
//! | ||
//! ``` | ||
//! use std::rc::Rc; | ||
//! | ||
//! let my_rc = Rc::new(()); | ||
//! let your_rc = Rc::clone(&my_rc); | ||
//! ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still waiting for this change, @camelid . (I don't like it but it appears to be the consensus.)
@camelid any updates on this? thanks |
Yeah, I've been meaning to get to this. |
3cdd7cb
to
b95e0c1
Compare
Starting with a rebase. |
I'm dropping the commit changing |
b95e0c1
to
e0eed3c
Compare
That recommendation was removed last year; there isn't a particular style that is officially recommended anymore.
@steveklabnik This is ready for a re-review :) |
Thank you! @bors: r+ rollup |
📌 Commit 4e30e10 has been approved by |
Also cleaned up some other small things.
@rustbot modify labels: T-doc