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

std.stdio: Add File.windowsHandle, fdopen and windowsHandleOpen #1888

Merged
1 commit merged into from Feb 16, 2014

Conversation

CyberShadow
Copy link
Member

https://d.puremagic.com/issues/show_bug.cgi?id=12142

This moves existing code from std.process to std.stdio for public use.

Cont. of #1877

@@ -69,6 +69,8 @@ version(Windows)
/+ Waiting for druntime pull 299
+/
extern (C) nothrow FILE* _wfopen(in wchar* filename, in wchar* mode);

import core.sys.windows.windows : HANDLE;
Copy link

Choose a reason for hiding this comment

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

I'm not a fan of this, due to that dreaded selective import bug. If I use WinAPI and then try to use stdio as well, I might just get a conflict of trying to use two different HANDLE definitions.

Copy link
Member Author

Choose a reason for hiding this comment

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

This works for me:

import win32.basetsd;
import std.stdio;

HANDLE h;

Maybe it was fixed?

Copy link

Choose a reason for hiding this comment

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

Hmm looks like it. Carry on.

This moves existing code from std.process to std.stdio for public use.
@CyberShadow
Copy link
Member Author

Fixed merge conflict.

@ghost
Copy link

ghost commented Feb 12, 2014

Anyone have more to say? @schveiguy @yebblies ?

@yebblies
Copy link
Member

All I've got is that this should probably have a linked bug report.

@CyberShadow
Copy link
Member Author

else _close(writeFD);
throw new StdioException("Cannot open pipe");
throw new StdioException("Error attaching pipe (" ~ e.msg ~ ")",
0);
Copy link

Choose a reason for hiding this comment

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

Why 0 instead of e.line?

Copy link
Member Author

Choose a reason for hiding this comment

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

The second parameter to StdioException is the errno, which is not valid at this point (it should have been processed by the caught exception and included in its message).

Copy link

Choose a reason for hiding this comment

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

Ah. But how about this:

auto ex = new StdioException(...);
ex.line = e.line;
throw ex;

IOW if there is any line info that's useful to us we could store it.

Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, how about we just modify .msg? Then file/line information will be preserved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, never mind, we need to throw a StdioException.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. There's no precedent for this, at least as far as StdioException goes. The exception is thrown in a dozen places, and none of those convey file/line information, even when that exception is thrown directly. IMHO it's better to fix this all at once separately, but since either way the exception information will always point to somewhere inside Phobos, there really isn't a strong motivation to do that. (The whole deal with passing file/line information across Exception constructors seems like a giant hack around subpar stack traces anyway.)

Copy link

Choose a reason for hiding this comment

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

Okie dokie.

@ghost
Copy link

ghost commented Feb 16, 2014

Looks good. Will wait for reply to #1888 (comment) before automerging.

@ghost
Copy link

ghost commented Feb 16, 2014

Auto-merge toggled on

ghost pushed a commit that referenced this pull request Feb 16, 2014
std.stdio: Add File.windowsHandle, fdopen and windowsHandleOpen
@ghost ghost merged commit fe532ec into dlang:master Feb 16, 2014
@CyberShadow
Copy link
Member Author

Thanks!

Throws: $(D ErrnoException) in case of error.
*/
version(StdDdoc)
void windowsHandleOpen(HANDLE handle, in char[] stdioOpenmode);
Copy link
Member

Choose a reason for hiding this comment

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

This breaks building documentation on non-Windows platforms, because HANDLE isn't defined.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants