Skip to content

Minor rewrite of env::current_exe docs; clarify symlinks. #46987

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

Merged
merged 1 commit into from
Jan 6, 2018
Merged
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
19 changes: 11 additions & 8 deletions src/libstd/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -571,8 +571,11 @@ pub fn temp_dir() -> PathBuf {

/// Returns the full filesystem path of the current running executable.
///
/// The path returned is not necessarily a "real path" of the executable as
/// there may be intermediate symlinks.
/// # Platform-specific behavior
///
/// If the executable was invoked through a symbolic link, some platforms will
/// return the path of the symbolic link and other platforms will return the
/// path of the symbolic link’s target.
///
/// # Errors
///
Expand All @@ -599,24 +602,24 @@ pub fn temp_dir() -> PathBuf {
/// Ok("/home/alex/foo")
/// ```
///
/// And you make a symbolic link of the program:
/// And you make a hard link of the program:
///
/// ```bash
/// $ ln foo bar
/// ```
///
/// When you run it, you won't get the original executable, you'll get the
Copy link
Contributor

Choose a reason for hiding this comment

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

Was the change from ' to deliberate? (Not sure what the convention is here).

Copy link
Member Author

Choose a reason for hiding this comment

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

deliberate but not something i feel strongly about. figure the line is already changing so i'm not ruining the diff in any way, but i'm fine keeping as-is

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind much either way however it looks like ' is used a lot more than in the existing doc comments, so my personal vote would be to keep ' for consistency (but feel free to keep this as-is to get a second opinion).

Copy link
Member

Choose a reason for hiding this comment

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

is the preferred symbol to use for apostrophe in english text according to the Unicode standard. ' is more commonplace because people can’t always be bothered to type Unicode symbols.

My personal vote is to follow the preferences set forth by the standard (I myself try to type unicode ), but is not something I would bug other people about whether they do or don’t follow the practice.

Copy link
Member

Choose a reason for hiding this comment

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

In all the docs i've written, i've just used the regular ' because is impossible to type on US-layout keyboards without a software compose key or other kind of software text replacement. On the other hand, the difference is likely too small to notice unless it was inconsistent within a single page. So here it's probably fine, but i wouldn't go using it on other pages without grabbing all the others on the same page like you did here. (And then possibly creating a tidy lint for it so we can keep on top of it. >_>)

/// symlink:
/// When you run it, you wont get the path of the original executable, you’ll
/// get the path of the hard link:
///
/// ```bash
/// $ ./bar
/// Ok("/home/alex/bar")
/// ```
///
/// This sort of behavior has been known to [lead to privilege escalation] when
/// used incorrectly, for example.
/// used incorrectly.
///
/// [lead to privilege escalation]: http://securityvulns.com/Wdocument183.html
/// [lead to privilege escalation]: https://securityvulns.com/Wdocument183.html
///
/// # Examples
///
Expand All @@ -625,7 +628,7 @@ pub fn temp_dir() -> PathBuf {
///
/// match env::current_exe() {
/// Ok(exe_path) => println!("Path of this executable is: {}",
/// exe_path.display()),
/// exe_path.display()),
/// Err(e) => println!("failed to get current exe path: {}", e),
/// };
/// ```
Expand Down