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

Revert conditional setting of stdin handle #3379

Merged
merged 6 commits into from
Aug 12, 2022
Merged

Revert conditional setting of stdin handle #3379

Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4f734d0
Revert 3121353c6ab2fbc5fced28f075c7fc45b53b661e
AceofSpades5757 Aug 10, 2022
5376926
Merge branch 'master' of github.com:helix-editor/helix into fix-3374
AceofSpades5757 Aug 11, 2022
f503a3f
Switch to conditional compilation
AceofSpades5757 Aug 11, 2022
403cf29
Run formatter
AceofSpades5757 Aug 11, 2022
d183407
Merge branch 'helix-editor:master' into fix-3374
AceofSpades5757 Aug 12, 2022
038376b
Switch from conditional compilation to compile-time bool
AceofSpades5757 Aug 12, 2022
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
5 changes: 5 additions & 0 deletions helix-term/src/commands.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4559,9 +4559,14 @@ fn shell_impl(
.stdout(Stdio::piped())
.stderr(Stdio::piped());

#[cfg(not(target_family = "windows"))]
if input.is_some() {
process.stdin(Stdio::piped());
}
#[cfg(target_family = "windows")]
{
process.stdin(Stdio::piped());
}
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need conditional compiling since it compiles ok on and off windows. The runtime behavior should be influenced by it though and that can be checked with the cfg! macro

if input.is_some() || cfg!(windows) {
    precss.stdin(Stdio::piped());
}

Copy link
Contributor Author

@AceofSpades5757 AceofSpades5757 Aug 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Is this usually preferable?

Copy link
Member

Choose a reason for hiding this comment

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

From what I understand, it's best to only use what conditional compilation you really need. That way you don't mistakenly end up with wildly different code-paths on different operating systems. At the very least it makes rust-analyzer a little quieter to keep conditional compiles to a minimum :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha, gotcha. Thanks!

I did have an issue with my first attempt, even though I was pretty sure it wasn't going to work. It did compile, but the binary would just hang there. The most surprising thing is that this was legal, and it compiled.

#[cfg(target_family = "windows")]
let a = "hello";


let mut process = match process.spawn() {
Ok(process) => process,
Expand Down