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.

Sign up for GitHub

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

Jump to bottom

Add Default implementation for &CStr, CString, Path #32990

Merged
merged 1 commit into from
May 5, 2016
Merged

Add Default implementation for &CStr, CString, Path #32990

merged 1 commit into from
May 5, 2016

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Apr 15, 2016

No description provided.

@rust-highfive
Copy link
Collaborator

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

@tbu-
Copy link
Contributor Author

tbu- commented Apr 15, 2016

Split off from #32785. Some motivation: #32785 (comment).

@alexcrichton alexcrichton mentioned this pull request Apr 15, 2016
Merged
@alexcrichton
Copy link
Member

CString seems fine to me personally as CString::new is the "clear and obvious constructor", the others seem conventionally fine but not perhaps well motivated.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Apr 15, 2016
@tbu-
Copy link
Contributor Author

tbu- commented Apr 15, 2016

"" is the default value for &str, I'd argue that the equivalent value should be the default value for &CStr.

GuillaumeGomez
GuillaumeGomez reviewed Apr 15, 2016
View reviewed changes
src/libstd/ffi/c_str.rs
@@ -326,6 +326,22 @@ impl fmt::Debug for CStr {
}
}

#[stable(feature = "cstr_default", since = "1.9.0")]
Copy link
Member

Choose a reason for hiding this comment

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

We just hit 1.8.0 release, so it becomes since = 1.10.0 (if you could confirm @alexcrichton?).

@nagisa
Copy link
Member

nagisa commented Apr 15, 2016

I’m not sure about "" as the default path. You can’t even do anything sensible with it (at least on linux), other than join to other paths.

@tbu-
Copy link
Contributor Author

tbu- commented Apr 15, 2016

Dunno, maybe base.join(config_option.unwrap_or_default())

@nagisa
Copy link
Member

nagisa commented Apr 15, 2016 •
edited
Loading

@tbu- for that purpose an equivalent of UNIX’s . has the same behaviour.

EDIT: in other words, I think this is too much a bikesheddable part to be included without an RFC.

@tbu-
Copy link
Contributor Author

tbu- commented Apr 16, 2016

@nagisa Not for verbatim paths on Windows.

@aturon
Copy link
Member

aturon commented Apr 19, 2016

I'm fine with the CString-related defaults, but agree that the one for Path is probably more problematic than it's worth.

@brson brson added the relnotes Marks issues that should be documented in the release notes of the next release. label May 4, 2016
@alexcrichton
Copy link
Member

The libs team discussed this PR during triage today and the conclusion was that the Default implementations for CStr and CString seem fine, but as mentioned previously we'd prefer that Path were left out for now.

Could you make those changes @tbu- and I'll r+? Thanks again for the PR!

@tbu- tbu- force-pushed the pr_more_defaults_cstr_path branch from 44e67d0 to 5efe083 Compare May 5, 2016 08:57
@tbu-
Copy link
Contributor Author

tbu- commented May 5, 2016

@alexcrichton Changed.

@alexcrichton
Copy link
Member

@bors: r+ 5efe083

@bors
Copy link
Contributor

bors commented May 5, 2016

⌛ Testing commit 5efe083 with merge 77987ba...

bors added a commit that referenced this pull request May 5, 2016
@bors
Auto merge of #32990 - tbu-:pr_more_defaults_cstr_path, r=alexcrichton
77987ba 
Add `Default` implementation for `&CStr`, `CString`, `Path`
@bors
Copy link
Contributor

bors commented May 5, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-32cross-opt, auto-linux-64-cargotest, auto-linux-64-cross-armhf, auto-linux-64-cross-armsf, auto-linux-64-cross-freebsd, auto-linux-64-cross-netbsd, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-64-opt-rustbuild, auto-mac-cross-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-cross-opt, auto-win-msvc-32-opt, auto-win-msvc-64-cargotest, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit 5efe083 into rust-lang:master May 5, 2016
steveklabnik added a commit that referenced this pull request Jul 7, 2016
@steveklabnik
Fix release notes for 1.10
d04e34b 
Path was not actually given a default impl in #32990, even though the PR title said it did.

r? @brson
@steveklabnik steveklabnik mentioned this pull request Jul 7, 2016
Merged
@tbu- tbu- mentioned this pull request Jan 28, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@aturon aturon

Labels
relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tbu- @rust-highfive @alexcrichton @nagisa @aturon @bors @GuillaumeGomez @brson