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

Improve compile error when using std.io.getStdOutHandle #19891

Closed
wants to merge 4 commits into from

Conversation

segfault-enjoyer
Copy link

After noticing that there are >4 issues made by people thinking this is a bug I thought it might be useful to improve the compile error to avoid more redundant issues. I would have liked to open a proposal issue for this, but that's not possible as of now. It is, however not breaking any code that isn't already broken, so I decided to make a pr anyways.

Related:
#19864
#17186
#6845
#14203

After noticing that there are >4 issues made by people thinking this is a bug I thought it might be useful to improve the compile error to avoid more redundant issues. I would have liked to open a proposal issue for this, but that's not possible as of now. It is, however not breaking any code that isn't already broken, so I decided to make a pr anyways.
@Vexu
Copy link
Member

Vexu commented May 8, 2024

Wouldn't hurt to also add this for at least stderr.

@segfault-enjoyer
Copy link
Author

Good call, I don't know why I didn't think of that

@segfault-enjoyer
Copy link
Author

Done 👍

@poypoyan
Copy link
Contributor

poypoyan commented May 8, 2024

Hi! Apologies for this: stdin is also affected. Up to you all if you'll add this.

I'll just note here that in general, anything put in global that calls windows.peb() and windows.teb() (code) are affected. But since most that I found for this are already Windows specific, I don't think we should add the notice for these.

@segfault-enjoyer
Copy link
Author

segfault-enjoyer commented May 8, 2024

I agree that the error shouldn't be returned by peb and teb, because this would also mean that the error message might have to be more generic, which would also make it less clear to the user if this is intended behavior or not. Additionally, this would not work for the current aarch64 workaround https://github.com/ziglang/zig/blob/master/lib/std/io.zig#L19 and cause more inconsistent behaveior from a user perspective. Let me know your thoughts on this!

@andrewrk
Copy link
Member

andrewrk commented May 9, 2024

No thanks, it's fine as-is. It also breaks something that works fine on non-Windows targets.

@andrewrk andrewrk closed this May 9, 2024
@Vexu
Copy link
Member

Vexu commented May 10, 2024

It also breaks something that works fine on non-Windows targets.

The errors are in Windows only branches.

@andrewrk
Copy link
Member

I see. It's still fine as-is. @inComptime is not meant to be used this way.

However, there is documentation missing on this function.

@segfault-enjoyer
Copy link
Author

You're right, I just checked the docs on @inComptime and it does say that it should be used primarily for e.g. performance improvements.

Should I just add the note about the functions not compiling on windows as a doc comment and open a new pr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants