-
Notifications
You must be signed in to change notification settings - Fork 13k
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
std: Improve pipe functionality #14781
Conversation
out: 0}; | ||
assert_eq!(libc::pipe(&mut fds.input), 0); | ||
return Pipe {input: fds.input, out: fds.out}; | ||
pub unsafe fn pipe() -> IoResult<Pipe> { |
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.
Maybe this could be raw_pipe
or something?
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.
Also, this function seems quite unsafe, and very much a low-level building block for io
, not really an os
thing. (I.e. would it be better to be in std::io::pipe
with the rest of the pipe stuff) shrug
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.
That's probably not a bad idea. Something like std::io::pipe::raw()
?
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.
Although then again the std::io
world is one of I/O objects, not of file descriptors, which this is diverging from... I think in generally I'm just not too enthused about exposing this.
Could this handle #9458 too? (Travis failed.) |
let os::Pipe { input, out } = match unsafe { os::pipe() } { | ||
Ok(p) => p, | ||
Err(io::IoError { detail, .. }) => return Err(IoError { | ||
code: ERROR as uint, |
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 is this overwriting the error code? Wouldn't the internal pipe
call know the most about why/what failed?
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.
This was done out of a desire to not write the windows/unix pipe() implementation in both librustuv and libnative. By having it in libstd, however, it needs to convert the errors a few times (pretty sub-par).
I'm not super happy with this though. The current process-spawning api also deals with file descriptors which is one of the reasons I left the os::pipe
api in libstd rather than removing it entirely.
Hm, I may need to go back to the drawing board for all of this...
The fields could probably be named better, I documented them awhile back with what they're clearly supposed to be doing for this purpose (I always forget as well). I think I'll just rename them to reader/writer like the |
* os::pipe() now returns IoResult<os::Pipe> * os::pipe() is now unsafe because it does not arrange for deallocation of file descriptors * os::Pipe fields are renamed from input to reader and out to write. * PipeStream::pair() has been added. This is a safe method to get a pair of pipes. * Dealing with pipes in native process bindings have been improved to be more robust in the face of failure and intermittent errors. This converts a few fail!() situations to Err situations. Closes rust-lang#9458 cc rust-lang#13538 Closes rust-lang#14724 [breaking-change]
* os::pipe() now returns `IoResult<os::Pipe>` * os::pipe() is now unsafe because it does not arrange for deallocation of file descriptors * PipeStream::pair() has been added. This is a safe method to get a pair of pipes. * Dealing with pipes in native process bindings have been improved to be more robust in the face of failure and intermittent errors. This converts a few fail!() situations to Err situations. cc #13538 Closes #14724 [breaking-change]
…=Veykril Introduce macro sub-namespaces and `macro_use` prelude This PR implements two mechanisms needed for correct macro name resolution: macro sub-namespace and `macro_use` prelude. - [macro sub-namespaces][subns-ref] Macros have two sub-namespaces: one for function-like macro and the other for those in attributes (including custom derive macros). When we're resolving a macro name for function-like macro, we should ignore non-function-like macros, and vice versa. This helps resolve single-segment macro names because we can (and should, as rustc does) fallback to names in preludes when the name in the current module scope is in different sub-namespace. - [`macro_use` prelude][prelude-ref] `#[macro_use]`'d extern crate declarations (including the standard library) bring their macros into scope, but they should not be prioritized over local macros (those defined in place and those explicitly imported). We have been bringing them into legacy (textual) macro scope, which has the highest precedence in name resolution. This PR introduces the `macro_use` prelude in crate-level `DefMap`s, whose precedence is lower than local macros but higher than the standard library prelude. The first 3 commits are drive-by fixes/refactors. Fixes rust-lang#8828 (prelude) Fixes rust-lang#12505 (prelude) Fixes rust-lang#12734 (prelude) Fixes rust-lang#13683 (prelude) Fixes rust-lang#13821 (prelude) Fixes rust-lang#13974 (prelude) Fixes rust-lang#14254 (namespace) [subns-ref]: https://doc.rust-lang.org/reference/names/namespaces.html#sub-namespaces [prelude-ref]: https://doc.rust-lang.org/reference/names/preludes.html#macro_use-prelude
…=Veykril Expand more single ident macro calls upon item collection Addresses rust-lang/rust-analyzer#14781 (comment) I believe this (almost) brings the number of unresolved names back to pre-rust-lang#14781: |r-a version|`analysis-stats compiler/rustc` (rust-lang/rust@69fef92) | |---|---| |pre-rust-lang#14781 (b069eb7) | exprs: 2747778, ??ty: 122236 (4%), ?ty: 107826 (3%), !ty: 728 | | rust-lang#14781 (a7944a9) | exprs: 2713080, ??ty: 139651 (5%), ?ty: 114444 (4%), !ty: 730 | | with this fix | exprs: 2747871, ??ty: 122237 (4%), ?ty: 108171 (3%), !ty: 676 | (I haven't investigated on the increase in some numbers but hopefully not too much of a problem) This is only a temporary solution. The core problem is that we haven't fully implemented the textual scope of legacy macros. For example, we *have been* failing to resolve `foo` in the following snippet, even before rust-lang#14781 or after this patch. As noted in a FIXME, we need a way to resolve names in textual scope without eager expansion during item collection. ```rust //- /main.rs crate:main deps:lib lib::mk_foo!(); const A: i32 = foo!(); //^^^^^^ unresolved-macro-call //- /lib.rs crate:lib #[macro_export] macro_rules! mk_foo { () => { macro_rules! foo { () => { 42 } } } } ```
IoResult<os::Pipe>
descriptors
pipes.
robust in the face of failure and intermittent errors. This converts a few
fail!() situations to Err situations.
cc #13538
Closes #14724
[breaking-change]