Closed k-sareen closed 1 year ago
I was being a bit miserly in terms of creating garbage so I added the packet
field to recvfrom()
even though it's not strictly necessary. If you provide a packet, it will override its values instead of allocating a new object.
Sorry for being AWOL. Been really busy with work. Two questions:
int
directly into the enum
variant that it corresponds to? Or on a higher-level, how would I convert between one enum
(Linux errnos) to a more descriptive and general enum
(for Network errors) where the conversion should be a bit shift operation between tags.match
statement feels a bit too verbose though.IntPointer
type (which is now private), I now return the actual int
field. My question is the compiler won't optimize the pointer load to a (potentially) stale register value, right? C uses the volatile
keyword to prevent this, but I'm unsure how Virgil manages this.@titzer Couple of things:
NetResult
type is extremely verbose. Main issues are: (i) Have to use unqualified case (i.e. x: Case
) as case-expression has type parameter; and (ii) Have to convert from NetResult<T>
to NetResult<Y>
even though NetResult.Error
is the same type (I've added comments in Socket.open()
which mention this).Range<byte>
as Ptr.atContents
and a bunch of other functions (such as System.fileWriteK
) are only defined for Array<byte>
. I think changing everything to accept a Range
type is out-of-scope of this PR. We can figure it out in the future.IntPointer
class. See Net.accept
for an example.socket.open()
before they perform any socket operation.InvalidInput
instead of NetError.InvalidInput
?EDIT: Here's an example of a somewhat descriptive error. The client is trying to send a message to a server that is not alive:
$ ./TcpEchoClient -v6 "No server"
Hello! Socket opened on fd 3
!Error: Socket connection failed: Connection refused
in main() [TcpEchoClient.v3 @ 44:37]
@titzer if you had time, could you review this PR as well? Sorry for being AWOL, got busy with work.
- Using the
NetResult
type is extremely verbose. Main issues are: (i) Have to use unqualified case (i.e.x: Case
) as case-expression has type parameter; and (ii) Have to convert fromNetResult<T>
toNetResult<Y>
even thoughNetResult.Error
is the same type (I've added comments inSocket.open()
which mention this).
I hear ya, I see it, and it does not look pretty :-)
One suggestion: you could add a method to NetResult
:
type NetResult<T> {
case Ok(val: T);
case Error(msg: string);
def force(f: string -> void) -> T {
match (this) {
Ok(val) => return val;
Error(msg) => f(msg);
}
var d: T;
return d;
}
}
def fatal = System.error("SocketError", _);
var x = someSocketCall().force(fatal); // unpacks upon success, calls {fatal} otherwise
- I couldn't change the API to use
Range<byte>
asPtr.atContents
and a bunch of other functions (such asSystem.fileWriteK
) are only defined forArray<byte>
. I think changing everything to accept aRange
type is out-of-scope of this PR. We can figure it out in the future.
Ah, true. Pointer.atContents
will have to be upgraded.
- I'm trusting the compiler won't optimize a pointer load to a potentially stale (register) value with the
IntPointer
class. SeeNet.accept
for an example.
That part still feels clunky. Allocating an object to store just an int
(and passing a pointer to the field, as opposed to a pointer into an array) is somewhat dangerous; that will need pinning too. But the answer is that the compiler backs off optimizations for fields that have had their address taken with Pointer.atField
.
- I've made the API such that a user has to call
socket.open()
before they perform any socket operation.- In general it gets pretty tiring to write every type/enum variant fully qualified. Is there a way to directly use, for example,
InvalidInput
instead ofNetError.InvalidInput
?
There is a match
statement variant that unpacks the cases; see above.
- Using the
NetResult
type is extremely verbose. Main issues are: (i) Have to use unqualified case (i.e.x: Case
) as case-expression has type parameter; and (ii) Have to convert fromNetResult<T>
toNetResult<Y>
even thoughNetResult.Error
is the same type (I've added comments inSocket.open()
which mention this).I hear ya, I see it, and it does not look pretty :-)
One suggestion: you could add a method to
NetResult
:type NetResult<T> { case Ok(val: T); case Error(msg: string); def force(f: string -> void) -> T { match (this) { Ok(val) => return val; Error(msg) => f(msg); } var d: T; return d; } } def fatal = System.error("SocketError", _); var x = someSocketCall().force(fatal); // unpacks upon success, calls {fatal} otherwise
Ah yes. I was thinking an equivalent to Rust's .expect()
/.unwrap()
would be useful too. Yeah it definitely looks cleaner. Will refactor it.
- I'm trusting the compiler won't optimize a pointer load to a potentially stale (register) value with the
IntPointer
class. SeeNet.accept
for an example.That part still feels clunky. Allocating an object to store just an
int
(and passing a pointer to the field, as opposed to a pointer into an array) is somewhat dangerous; that will need pinning too. But the answer is that the compiler backs off optimizations for fields that have had their address taken withPointer.atField
.
What would the difference be between an Array<int>
and an class IntPointer
? From the perspective of the GC, they should both be pinned and on the heap, no?
- I've made the API such that a user has to call
socket.open()
before they perform any socket operation.- In general it gets pretty tiring to write every type/enum variant fully qualified. Is there a way to directly use, for example,
InvalidInput
instead ofNetError.InvalidInput
?There is a
match
statement variant that unpacks the cases; see above.
Ah I see. So the cases are only inferred in a match (this)
? What about when I just want to refer to a specific variant of an enum
without fully qualifying it, as with NetError.*
?
Also are you happy with the current definition of NetResult
or should I adapt it to be similar to what you have in your example where the Error
variant has as string
field instead of a NetError
field.
What would the difference be between an
Array<int>
and anclass IntPointer
? From the perspective of the GC, they should both be pinned and on the heap, no?
It's just more code to define a new class. The right solution in the end will be to define a struct
and use it as a view over an Array<byte>
. I'd prefer an Array<int>
if for no other reason than it's shorter.
- I've made the API such that a user has to call
socket.open()
before they perform any socket operation.- In general it gets pretty tiring to write every type/enum variant fully qualified. Is there a way to directly use, for example,
InvalidInput
instead ofNetError.InvalidInput
?There is a
match
statement variant that unpacks the cases; see above.Ah I see. So the cases are only inferred in a
match (this)
? What about when I just want to refer to a specific variant of anenum
without fully qualifying it, as withNetError.*
?
It doesn't need to be this
, any expression that is of a variant type can be matched over with deconstruction.
Also are you happy with the current definition of
NetResult
or should I adapt it to be similar to what you have in your example where theError
variant has asstring
field instead of aNetError
field.
I think it's OK to land now; sorry I've been so pedantic. At some point I'll write some code against the API to get a better feel for it.
What would the difference be between an
Array<int>
and anclass IntPointer
? From the perspective of the GC, they should both be pinned and on the heap, no?It's just more code to define a new class. The right solution in the end will be to define a
struct
and use it as a view over anArray<byte>
. I'd prefer anArray<int>
if for no other reason than it's shorter.
Right -- that makes sense. I'll change it to an Array<int>
.
Ah I see. So the cases are only inferred in a
match (this)
? What about when I just want to refer to a specific variant of anenum
without fully qualifying it, as withNetError.*
?It doesn't need to be
this
, any expression that is of a variant type can be matched over with deconstruction.
Sorry, not sure I follow? I had previously tried to do that with NetResult
in some functions and the compiler gave me an error about having to use an unqualified case. I guess that error has to do more with the type parameter?
Also are you happy with the current definition of
NetResult
or should I adapt it to be similar to what you have in your example where theError
variant has asstring
field instead of aNetError
field.I think it's OK to land now; sorry I've been so pedantic. At some point I'll write some code against the API to get a better feel for it.
Yeah I'm not too happy with the UX of the API right now, but the majority of the issue is just the verbosity, so maybe it'll be fixed when I add the .force()
function/when the ?
operator is added for error propagation.
@titzer So I've added the above API, which definitely cleans up the user-facing API significantly. However, the library internals are still very messy due to the aforementioned mentioned reasons:
Using the NetResult type is extremely verbose. Main issues are: (i) Have to use unqualified case (i.e. x: Case) as case-expression has type parameter; and (ii) Have to convert from NetResult\
to NetResult\ even though NetResult.Error is the same type (I've added comments in Socket.open() which mention this).
I think the type system should be able to deal with the second issue as there is no real reason the conversion is not possible. Not sure how to deal with the first issue. Technically the generic parameter T
should be easily inferred since it's in the return type. So maybe the type system just has to be updated to be able to infer the generic type there.
This commit makes the error checking and handling more robust and adds support for connectionless and connected UDP sockets.