-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add Crystal::System::FileHandle #4912
Conversation
Crystal::System::FileHandle represents a handle to a file, or file-like object (pipe, tty, etc.).
@@ -21,15 +21,15 @@ class IO::FileDescriptor | |||
# This will prevent displaying back to the user what they enter on the terminal. | |||
# Only call this when this IO is a TTY, such as a not redirected stdin. | |||
def noecho! | |||
if LibC.tcgetattr(fd, out mode) != 0 | |||
if LibC.tcgetattr(@handle.platform_specific, out mode) != 0 |
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.
A lot of this stuff still needs to be platform abstracted.
One thing I'm concerned about is that we've separated |
It's unclear for me about
So, what is the purpose of making |
@txe By that comment I simply meant that |
@RX14 It would be great if you could finish this work. It will really help me with adding functionality for windows. |
Maybe |
I don't think that changing the class hierarchy based on platform will turn out well... |
It could be an included module, just an implementation detail. |
@asterite Except my concern is that we have to have file descriptors which are neither sockets or files. Modules can't be instantiated by their own so that doesn't solve much. |
I mean, File, Socket and pipes could include IO::FileDescriptor. Or Socket in Windows doesn't inherit IO::FileDescriptor. In any case I don't think a different hierarchy is a problem, with an IO you want to read and write. |
Well most of the the role of |
That's what I'm saying. |
@asterite The problem with that proposal is that it means that I think this PR is probably the best we can do for now. |
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.
I can't say whether this change is good or not. There are no Windows example to review and balance the changes and decide.
IMHO IO::FileDescriptor
is left spineless. It delegates everything to Crystal::System::FileHandle
and does nothing. Why bother keeping it?
Why the Crystal::System::FileHandle
naming? It's only a handle in 1 case (Windows) and a descriptor for all other targets. It's not even a file, neither a file handle on Windows.
I think I'd go with @asterite change to make IO a class, first, then have a Crystal::System::IO
module and IO::System
class:
module Crystal::System::IO
# actual impl
end
class IO::System < IO
# documented, empty, methods
include Crystal::System::IO
end
class File < IO::System; end
class Socket < IO::System; end
I would also have system specific namings (i.e. fd
or handle
) and a generic #to_unsafe
to access either one.
# Returns the platform-specific representation of this handle. | ||
# | ||
# On unix-like platforms this returns an Int32 file descriptor. | ||
# def platform_specific |
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.
platform_specific
is the worst possible naming. If it's supposed to represent the underlying system data, then maybe raw
or even handle
if we go with windows naming (bad idea: Windows is one target, POSIX is all others) or just a plain #to_unsafe
so we can pass @handle
directly to C functions instead of the horrifying @handle.platform_specific
we see everywhere.
In fact, I don't even see any reason to abstract the naming here: any POSIX system specifics will expect a file descriptor, and Windows system specifics will expect a HANDLE.
Trying to abstrtact this is actually confusing and misleading. We could have the false impression that LibC.tcsetattr(@handle.platform_specific, ...)
will just work on Windows for example.
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.
I think you're right that it's bad naming, but I don't think that #to_unsafe
is a good way to handle this. It's way too magic to pass a crystal object and it magically gets converted into a file descriptor. I think automatically calling to_unsafe
is way too magic in general...
I think you're probably right about naming it fd
on posix and handle
on windows.
raise Errno.new("Could not reopen file descriptor") | ||
end | ||
{% else %} | ||
# dup doesn't copy the CLOEXEC flag, copy it manually to the new |
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.
Misplaced comment.
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.
I'm confused, it's missing a word but why is it misplaced? It's documenting the implementation so it should be inside the method.
I'm not sure we should be exposing
According to wikipedia, file descriptors are a type of handle. I agree with wikipedia's definition. |
I think we should merge #4901 before this PR though. |
Still from wikipedia:
@RX14 I think it's important to emphasis the fact that it is a resource handler, and not simply a generic handler, or a more precise one like file handler. I also agree on this definition of a resource handler, and how a FD, socket, PID, etc.. are also resource handlers. But in our case, I think it's a concept too generic to use in a programming language, and we should have classes for the abstraction level above, which we can actually implement using syscalls and stuff. |
Superseeded by #5333 |
Crystal::System::FileHandle represents a handle to a file, or file-like
object (pipe, tty, etc.).
The specs currently fail due to debug line-numbers being broken for some weird reason. I just wanted to PR this so people could criticize the technique.