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

Fix spacing of links in inline code. #88343

Merged
merged 1 commit into from
Sep 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 12 additions & 16 deletions library/alloc/src/fmt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,7 @@
//! provides some helper methods.
//!
//! Additionally, the return value of this function is [`fmt::Result`] which is a
//! type alias of [`Result`]`<(), `[`std::fmt::Error`]`>`. Formatting implementations
//! type alias of <code>[Result]<(), [std::fmt::Error]></code>. Formatting implementations
//! should ensure that they propagate errors from the [`Formatter`] (e.g., when
//! calling [`write!`]). However, they should never return errors spuriously. That
//! is, a formatting implementation must and may only return an error if the
Expand Down Expand Up @@ -505,23 +505,19 @@
//! it would internally pass around this structure until it has been determined
//! where output should go to.
//!
//! [`fmt::Result`]: Result
//! [`Result`]: core::result::Result
//! [`std::fmt::Error`]: Error
//! [`write!`]: core::write
//! [`write`]: core::write
//! [`format!`]: crate::format
//! [`to_string`]: crate::string::ToString
//! [`writeln!`]: core::writeln
//! [`fmt::Result`]: Result "fmt::Result"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default this won't have a tooltip, right? Why did you add one?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, links generated by rustdoc do have a tooltip.

E.g. something like

/// Talking about [`Result`][std::fmt::Result] here!
pub mod m {}

will come with a tooltip

Screenshot_20210906_223756

Whereas (unfortunately) the following alternative

/// Talking about [`Result`] here!
/// 
/// [`Result`]: std::fmt::Result
pub mod m {}

creates almost the same documentation, just lacking the tooltip. That’s the general reason why I’ve converted many instances of

[foo]: path

into

[foo]: path "path"

and in some cases, I’ve also adjusted the tooltip slightly to make it more easy to understand. This includes the case here because Result is part of the prelude, a simple "Result" tooltip seems confusing to me, the std:: prefix is something I’ve often avoided for tooltips OTOH (not least because the paths themselves also often are relative so some imported std::foo module).


Also, as mentioned in the description of this PR, the addition of tooltips isn’t systematic and only covers places that I happened to come across. (Furthermore, the amount of tooltips added declines from earlier to later commits.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I think this would be better to fix systematically in rustdoc rather than adding it piecemeal to individual links. Could you open an issue and revert the tooltips you've added this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"revert" as in “add another commit to revert them” or as a “rebase / edit / force push”. Because the former could easily happen in a later PR, too; these tooltips aren't particularly hard to find (and remove) with a regex search.

Either way, the revert would only remove those cases where the path and tooltip look identical, right? E.g. the fmt::Result should better stay IMO, because I don't like a Result tooltip.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, no answer. I’m assuming that an additional commit is okay, and that only the ones where tooltip and path are identical need to be removed. This is more easily accomplished by a systematic find-and-replace that by looking through all my commits here. This could, as noted, easily be a separate PR, done after the relevant change to rustdoc was made, but I’m including a commit here now to get this PR going.

Copy link
Member

@jyn514 jyn514 Sep 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, I haven't had much time for rust lately.

Either way, the revert would only remove those cases where the path and tooltip look identical, right? E.g. the fmt::Result should better stay IMO, because I don't like a Result tooltip.

Well, ideally I wouldn't want any changes, because it makes them inconsistent and it will hopefully be done by rustdoc automatically eventually ... but if you feel strongly it seems fine to keep, it's just more work for me to review. I misunderstood - you're talking about showing core::result::Result links as std::result::Resultand that sort of thing. I guess that's fine? I think showing the link used in the source is more clear in a way but I don't feel strongly.

I’m assuming that an additional commit is okay, and that only the ones where tooltip and path are identical need to be removed.

Yup, that's fine - I don't have strong opinions about git commits.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think showing the link used in the source is more clear in a way but I don't feel strongly.

The problem I see is that the link used in the source code depends on what the imports are, and the imports depends on what happens to be used in the source code. If e.g. I’m writing documentation mentioning, say, mem::transmute, then I’d use the path men::transmute (resulting in a men::transmute tooltip) if the module I’m in does use core::mem (or use crate::mem). But if mem is only used in docs, AFAIK I can’t just add an import for that because (I think) it’ll trigger an unused imports lint and fail CI. But it’s IMO better to concistently have it have the tooltip mem::transmute and not sometimes core::mem::transmute or and crate::mem::transmute, etc…, depending on the implementation details (i.e. imports) of the module your documentation comes from (in particular, I don’t think the crate:: prefix is a good thing showing up in standard library tooltips).

//! [Result]: core::result::Result "std::result::Result"
//! [std::fmt::Error]: Error "fmt::Error"
//! [`write`]: write() "fmt::write"
//! [`to_string`]: crate::string::ToString::to_string "ToString::to_string"
//! [`write_fmt`]: ../../std/io/trait.Write.html#method.write_fmt
//! [`std::io::Write`]: ../../std/io/trait.Write.html
//! [`print!`]: ../../std/macro.print.html
//! [`println!`]: ../../std/macro.println.html
//! [`eprint!`]: ../../std/macro.eprint.html
//! [`eprintln!`]: ../../std/macro.eprintln.html
//! [`format_args!`]: core::format_args
//! [`fmt::Arguments`]: Arguments
//! [`format`]: crate::format
//! [`print!`]: ../../std/macro.print.html "print!"
//! [`println!`]: ../../std/macro.println.html "println!"
//! [`eprint!`]: ../../std/macro.eprint.html "eprint!"
//! [`eprintln!`]: ../../std/macro.eprintln.html "eprintln!"
//! [`fmt::Arguments`]: Arguments "fmt::Arguments"
//! [`format`]: format() "fmt::format"

#![stable(feature = "rust1", since = "1.0.0")]

Expand Down
16 changes: 4 additions & 12 deletions library/alloc/src/rc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -781,9 +781,7 @@ impl<T: ?Sized> Rc<T> {
/// Consumes the `Rc`, returning the wrapped pointer.
///
/// To avoid a memory leak the pointer must be converted back to an `Rc` using
/// [`Rc::from_raw`][from_raw].
///
/// [from_raw]: Rc::from_raw
/// [`Rc::from_raw`].
///
/// # Examples
///
Expand Down Expand Up @@ -834,7 +832,7 @@ impl<T: ?Sized> Rc<T> {
/// and alignment as `T`. This is trivially true if `U` is `T`.
/// Note that if `U` is not `T` but has the same size and alignment, this is
/// basically like transmuting references of different types. See
/// [`mem::transmute`][transmute] for more information on what
/// [`mem::transmute`] for more information on what
/// restrictions apply in this case.
///
/// The user of `from_raw` has to make sure a specific value of `T` is only
Expand All @@ -844,7 +842,6 @@ impl<T: ?Sized> Rc<T> {
/// even if the returned `Rc<T>` is never accessed.
///
/// [into_raw]: Rc::into_raw
/// [transmute]: core::mem::transmute
///
/// # Examples
///
Expand Down Expand Up @@ -1086,8 +1083,6 @@ impl<T: ?Sized> Rc<T> {
/// assert!(Rc::ptr_eq(&five, &same_five));
/// assert!(!Rc::ptr_eq(&five, &other_five));
/// ```
///
/// [`ptr::eq`]: core::ptr::eq
pub fn ptr_eq(this: &Self, other: &Self) -> bool {
this.ptr.as_ptr() == other.ptr.as_ptr()
}
Expand Down Expand Up @@ -1993,7 +1988,7 @@ impl<T, I: iter::TrustedLen<Item = T>> ToRcSlice<T> for I {

/// `Weak` is a version of [`Rc`] that holds a non-owning reference to the
/// managed allocation. The allocation is accessed by calling [`upgrade`] on the `Weak`
/// pointer, which returns an [`Option`]`<`[`Rc`]`<T>>`.
/// pointer, which returns an <code>[Option]<[Rc]\<T>></code>.
///
/// Since a `Weak` reference does not count towards ownership, it will not
/// prevent the value stored in the allocation from being dropped, and `Weak` itself makes no
Expand Down Expand Up @@ -2090,7 +2085,7 @@ impl<T: ?Sized> Weak<T> {
/// // assert_eq!("hello", unsafe { &*weak.as_ptr() });
/// ```
///
/// [`null`]: core::ptr::null
/// [`null`]: ptr::null
#[stable(feature = "rc_as_ptr", since = "1.45.0")]
pub fn as_ptr(&self) -> *const T {
let ptr: *mut RcBox<T> = NonNull::as_ptr(self.ptr);
Expand Down Expand Up @@ -2317,8 +2312,6 @@ impl<T: ?Sized> Weak<T> {
/// let third = Rc::downgrade(&third_rc);
/// assert!(!first.ptr_eq(&third));
/// ```
///
/// [`ptr::eq`]: core::ptr::eq
#[inline]
#[stable(feature = "weak_ptr_eq", since = "1.39.0")]
pub fn ptr_eq(&self, other: &Self) -> bool {
Expand Down Expand Up @@ -2400,7 +2393,6 @@ impl<T> Default for Weak<T> {
/// Constructs a new `Weak<T>`, without allocating any memory.
/// Calling [`upgrade`] on the return value always gives [`None`].
///
/// [`None`]: Option
/// [`upgrade`]: Weak::upgrade
///
/// # Examples
Expand Down
48 changes: 25 additions & 23 deletions library/alloc/src/string.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ use crate::vec::Vec;
///
/// # Examples
///
/// You can create a `String` from [a literal string][`str`] with [`String::from`]:
/// You can create a `String` from [a literal string][`&str`] with [`String::from`]:
///
/// [`String::from`]: From::from
///
Expand Down Expand Up @@ -128,7 +128,7 @@ use crate::vec::Vec;
/// println!("The first letter of s is {}", s[0]); // ERROR!!!
/// ```
///
/// [`OsString`]: ../../std/ffi/struct.OsString.html
/// [`OsString`]: ../../std/ffi/struct.OsString.html "ffi::OsString"
///
/// Indexing is intended to be a constant-time operation, but UTF-8 encoding
/// does not allow us to do this. Furthermore, it's not clear what sort of
Expand All @@ -141,7 +141,7 @@ use crate::vec::Vec;
///
/// # Deref
///
/// `String`s implement [`Deref`]`<Target=str>`, and so inherit all of [`str`]'s
/// `String` implements <code>[Deref]<Target = [str]></code>, and so inherits all of [`str`]'s
/// methods. In addition, this means that you can pass a `String` to a
/// function which takes a [`&str`] by using an ampersand (`&`):
///
Expand Down Expand Up @@ -182,7 +182,7 @@ use crate::vec::Vec;
/// to explicitly extract the string slice containing the string. The second
/// way changes `example_func(&example_string);` to
/// `example_func(&*example_string);`. In this case we are dereferencing a
/// `String` to a [`str`][`&str`], then referencing the [`str`][`&str`] back to
/// `String` to a [`str`], then referencing the [`str`] back to
/// [`&str`]. The second way is more idiomatic, however both work to do the
/// conversion explicitly rather than relying on the implicit conversion.
///
Expand Down Expand Up @@ -282,9 +282,11 @@ use crate::vec::Vec;
///
/// Here, there's no need to allocate more memory inside the loop.
///
/// [`str`]: prim@str
/// [`&str`]: prim@str
/// [`Deref`]: core::ops::Deref
/// [str]: prim@str "str"
/// [`str`]: prim@str "str"
/// [`&str`]: prim@str "&str"
/// [Deref]: core::ops::Deref "ops::Deref"
/// [`Deref`]: core::ops::Deref "ops::Deref"
Copy link
Member

@jyn514 jyn514 Sep 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can avoid needing a second reference link by using [`Deref`][Deref]. But doesn't seem important to change.

/// [`as_str()`]: String::as_str
#[derive(PartialOrd, Eq, Ord)]
#[cfg_attr(not(test), rustc_diagnostic_item = "string_type")]
Expand All @@ -308,10 +310,10 @@ pub struct String {
/// an analogue to `FromUtf8Error`, and you can get one from a `FromUtf8Error`
/// through the [`utf8_error`] method.
///
/// [`Utf8Error`]: core::str::Utf8Error
/// [`std::str`]: core::str
/// [`&str`]: prim@str
/// [`utf8_error`]: Self::utf8_error
/// [`Utf8Error`]: str::Utf8Error "std::str::Utf8Error"
/// [`std::str`]: core::str "std::str"
/// [`&str`]: prim@str "&str"
/// [`utf8_error`]: FromUtf8Error::utf8_error
///
/// # Examples
///
Expand Down Expand Up @@ -487,8 +489,8 @@ impl String {
/// with this error.
///
/// [`from_utf8_unchecked`]: String::from_utf8_unchecked
/// [`Vec<u8>`]: crate::vec::Vec
/// [`&str`]: prim@str
/// [`Vec<u8>`]: crate::vec::Vec "Vec"
/// [`&str`]: prim@str "&str"
/// [`into_bytes`]: String::into_bytes
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
Expand Down Expand Up @@ -524,7 +526,7 @@ impl String {
/// it's already valid UTF-8, we don't need a new allocation. This return
/// type allows us to handle both cases.
///
/// [`Cow<'a, str>`]: crate::borrow::Cow
/// [`Cow<'a, str>`]: crate::borrow::Cow "borrow::Cow"
///
/// # Examples
///
Expand Down Expand Up @@ -625,7 +627,7 @@ impl String {
/// conversion requires a memory allocation.
///
/// [`from_utf8_lossy`]: String::from_utf8_lossy
/// [`Cow<'a, str>`]: crate::borrow::Cow
/// [`Cow<'a, str>`]: crate::borrow::Cow "borrow::Cow"
/// [U+FFFD]: core::char::REPLACEMENT_CHARACTER
///
/// # Examples
Expand Down Expand Up @@ -1721,11 +1723,11 @@ impl String {
unsafe { self.as_mut_vec() }.splice((start, end), replace_with.bytes());
}

/// Converts this `String` into a [`Box`]`<`[`str`]`>`.
/// Converts this `String` into a <code>[Box]<[str]></code>.
///
/// This will drop any excess capacity.
///
/// [`str`]: prim@str
/// [str]: prim@str "str"
///
/// # Examples
///
Expand Down Expand Up @@ -1795,8 +1797,8 @@ impl FromUtf8Error {
/// an analogue to `FromUtf8Error`. See its documentation for more details
/// on using it.
///
/// [`std::str`]: core::str
/// [`&str`]: prim@str
/// [`std::str`]: core::str "std::str"
/// [`&str`]: prim@str "&str"
///
/// # Examples
///
Expand Down Expand Up @@ -2319,7 +2321,7 @@ impl ops::DerefMut for String {
///
/// This alias exists for backwards compatibility, and may be eventually deprecated.
///
/// [`Infallible`]: core::convert::Infallible
/// [`Infallible`]: core::convert::Infallible "convert::Infallible"
#[stable(feature = "str_parse_error", since = "1.5.0")]
pub type ParseError = core::convert::Infallible;

Expand Down Expand Up @@ -2606,7 +2608,7 @@ impl<'a> From<&'a str> for Cow<'a, str> {
/// assert_eq!(Cow::from("eggplant"), Cow::Borrowed("eggplant"));
/// ```
///
/// [`Borrowed`]: crate::borrow::Cow::Borrowed
/// [`Borrowed`]: crate::borrow::Cow::Borrowed "borrow::Cow::Borrowed"
#[inline]
fn from(s: &'a str) -> Cow<'a, str> {
Cow::Borrowed(s)
Expand All @@ -2629,7 +2631,7 @@ impl<'a> From<String> for Cow<'a, str> {
/// assert_eq!(Cow::from(s), Cow::<'static, str>::Owned(s2));
/// ```
///
/// [`Owned`]: crate::borrow::Cow::Owned
/// [`Owned`]: crate::borrow::Cow::Owned "borrow::Cow::Owned"
#[inline]
fn from(s: String) -> Cow<'a, str> {
Cow::Owned(s)
Expand All @@ -2651,7 +2653,7 @@ impl<'a> From<&'a String> for Cow<'a, str> {
/// assert_eq!(Cow::from(&s), Cow::Borrowed("eggplant"));
/// ```
///
/// [`Borrowed`]: crate::borrow::Cow::Borrowed
/// [`Borrowed`]: crate::borrow::Cow::Borrowed "borrow::Cow::Borrowed"
#[inline]
fn from(s: &'a String) -> Cow<'a, str> {
Cow::Borrowed(s.as_str())
Expand Down
24 changes: 12 additions & 12 deletions library/alloc/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,8 @@ macro_rules! acquire {
/// first: after all, isn't the point of `Arc<T>` thread safety? The key is
/// this: `Arc<T>` makes it thread safe to have multiple ownership of the same
/// data, but it doesn't add thread safety to its data. Consider
/// `Arc<`[`RefCell<T>`]`>`. [`RefCell<T>`] isn't [`Sync`], and if `Arc<T>` was always
/// [`Send`], `Arc<`[`RefCell<T>`]`>` would be as well. But then we'd have a problem:
/// <code>Arc<[RefCell\<T>]></code>. [`RefCell<T>`] isn't [`Sync`], and if `Arc<T>` was always
/// [`Send`], <code>Arc<[RefCell\<T>]></code> would be as well. But then we'd have a problem:
/// [`RefCell<T>`] is not thread safe; it keeps track of the borrowing count using
/// non-atomic operations.
///
Expand Down Expand Up @@ -176,6 +176,7 @@ macro_rules! acquire {
/// [deref]: core::ops::Deref
/// [downgrade]: Arc::downgrade
/// [upgrade]: Weak::upgrade
/// [RefCell\<T>]: core::cell::RefCell
/// [`RefCell<T>`]: core::cell::RefCell
/// [`std::sync`]: ../../std/sync/index.html
/// [`Arc::clone(&from)`]: Arc::clone
Expand Down Expand Up @@ -206,7 +207,7 @@ macro_rules! acquire {
///
/// Sharing a mutable [`AtomicUsize`]:
///
/// [`AtomicUsize`]: core::sync::atomic::AtomicUsize
/// [`AtomicUsize`]: core::sync::atomic::AtomicUsize "sync::atomic::AtomicUsize"
///
/// ```no_run
/// use std::sync::Arc;
Expand Down Expand Up @@ -262,7 +263,7 @@ impl<T: ?Sized> Arc<T> {

/// `Weak` is a version of [`Arc`] that holds a non-owning reference to the
/// managed allocation. The allocation is accessed by calling [`upgrade`] on the `Weak`
/// pointer, which returns an [`Option`]`<`[`Arc`]`<T>>`.
/// pointer, which returns an <code>[Option]<[Arc]\<T>></code>.
///
/// Since a `Weak` reference does not count towards ownership, it will not
/// prevent the value stored in the allocation from being dropped, and `Weak` itself makes no
Expand Down Expand Up @@ -476,7 +477,7 @@ impl<T> Arc<T> {
/// assert_eq!(*zero, 0)
/// ```
///
/// [zeroed]: ../../std/mem/union.MaybeUninit.html#method.zeroed
/// [zeroed]: mem::MaybeUninit::zeroed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, did this move into core recently or something? Or was it just missed when converting links to intra-doc links?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea :-)

#[cfg(not(no_global_oom_handling))]
#[unstable(feature = "new_uninit", issue = "63291")]
pub fn new_zeroed() -> Arc<mem::MaybeUninit<T>> {
Expand Down Expand Up @@ -684,7 +685,7 @@ impl<T> Arc<[T]> {
/// assert_eq!(*values, [0, 0, 0])
/// ```
///
/// [zeroed]: ../../std/mem/union.MaybeUninit.html#method.zeroed
/// [zeroed]: mem::MaybeUninit::zeroed
#[cfg(not(no_global_oom_handling))]
#[unstable(feature = "new_uninit", issue = "63291")]
pub fn new_zeroed_slice(len: usize) -> Arc<[mem::MaybeUninit<T>]> {
Expand Down Expand Up @@ -712,7 +713,7 @@ impl<T> Arc<mem::MaybeUninit<T>> {
/// Calling this when the content is not yet fully initialized
/// causes immediate undefined behavior.
///
/// [`MaybeUninit::assume_init`]: ../../std/mem/union.MaybeUninit.html#method.assume_init
/// [`MaybeUninit::assume_init`]: mem::MaybeUninit::assume_init
///
/// # Examples
///
Expand Down Expand Up @@ -751,7 +752,7 @@ impl<T> Arc<[mem::MaybeUninit<T>]> {
/// Calling this when the content is not yet fully initialized
/// causes immediate undefined behavior.
///
/// [`MaybeUninit::assume_init`]: ../../std/mem/union.MaybeUninit.html#method.assume_init
/// [`MaybeUninit::assume_init`]: mem::MaybeUninit::assume_init
///
/// # Examples
///
Expand Down Expand Up @@ -1086,7 +1087,7 @@ impl<T: ?Sized> Arc<T> {
/// assert!(!Arc::ptr_eq(&five, &other_five));
/// ```
///
/// [`ptr::eq`]: core::ptr::eq
/// [`ptr::eq`]: core::ptr::eq "ptr::eq"
pub fn ptr_eq(this: &Self, other: &Self) -> bool {
this.ptr.as_ptr() == other.ptr.as_ptr()
}
Expand Down Expand Up @@ -1714,7 +1715,7 @@ impl<T: ?Sized> Weak<T> {
/// // assert_eq!("hello", unsafe { &*weak.as_ptr() });
/// ```
///
/// [`null`]: core::ptr::null
/// [`null`]: core::ptr::null "ptr::null"
#[stable(feature = "weak_into_raw", since = "1.45.0")]
pub fn as_ptr(&self) -> *const T {
let ptr: *mut ArcInner<T> = NonNull::as_ptr(self.ptr);
Expand Down Expand Up @@ -1806,7 +1807,6 @@ impl<T: ?Sized> Weak<T> {
/// [`new`]: Weak::new
/// [`into_raw`]: Weak::into_raw
/// [`upgrade`]: Weak::upgrade
/// [`forget`]: std::mem::forget
#[stable(feature = "weak_into_raw", since = "1.45.0")]
pub unsafe fn from_raw(ptr: *const T) -> Self {
// See Weak::as_ptr for context on how the input pointer is derived.
Expand Down Expand Up @@ -1982,7 +1982,7 @@ impl<T: ?Sized> Weak<T> {
/// assert!(!first.ptr_eq(&third));
/// ```
///
/// [`ptr::eq`]: core::ptr::eq
/// [`ptr::eq`]: core::ptr::eq "ptr::eq"
#[inline]
#[stable(feature = "weak_ptr_eq", since = "1.39.0")]
pub fn ptr_eq(&self, other: &Self) -> bool {
Expand Down
Loading