Conversation
dicej
left a comment
dicej
left a comment
There was a problem hiding this comment.
Thanks, @yowl.
Per my inline comments, I expect chunks of this will need to be revisited and/or rewritten once you start tackling more complicated test scenarios. Personally, I'd be fine with larger PRs that get more tests passing, with code that more closely resembles the final product.
I understand that you're trying to be incremental here, but I suspect it will be easier (for me and for you) to see how things fit together once we have tests like simple-import-params-results and simple-stream-payload working. Those tests involve a more realistic mix of data flow and control flow, and supporting them will help flesh out the skeleton of support you've built so far.
| public readonly uint Code; | ||
|
|
||
| public readonly EventCode EventCode => (EventCode)Raw; | ||
| public readonly WaitableStatus WaitableStatus; |
There was a problem hiding this comment.
This field seems redundant. Could it use a closure like EventCode and WaitableCount?
|
|
||
| tcs.SetResult(); | ||
| } | ||
| return CallbackCode.Exit; |
There was a problem hiding this comment.
FYI, we should only return CallbackCode.Exit here if the current task has nothing left to do. This is fine to get a simple test passing, but we'll soon need to change this for more complicated tests.
| } | ||
|
|
||
| // TODO join and complete the task somwhere. | ||
| TaskCompletionSource tcs = new TaskCompletionSource(); |
There was a problem hiding this comment.
I'd expect we'd insert into PendingCallbacks here (and ditto for the generic version below).
|
|
||
| // TODO: Generate per type for this instrinsic. | ||
| public Task Read() | ||
| public unsafe Task Read() |
There was a problem hiding this comment.
What are the implications of marking this unsafe? I.e. what must the caller ensure to use it safely?
| AsyncSupport.PendingCallbacks.TryAdd((IntPtr)contextTaskPtr, tcs); | ||
|
|
||
| // TODO: Defer dropping borrowed resources until a result is returned. | ||
| return (uint)CallbackCode.Yield; |
There was a problem hiding this comment.
I'd expect we'd return ((uint)CallbackCode.Wait) | (contextTaskPtr.Set) here or similar. Returning Yield is basically a busy wait.
|
I'm fine with whatever you think is best. Do you want me to address the comments and pull this PR to get those 2 extra tests passing? |
Yes, if it's not too much trouble. That would give me more confidence we're heading in the right direction. |
2 participants
This PR add support for C# for the async
pending-importruntime test. It moves the C# codegen away from minimalistCstyle to a more Go and Rust like approach. This PR gets this next test passing on the understanding there is still lots to do for C# async codegen. Apologies for the largeness of the PR, I hope the restructure is mostly complete here, and the next PR will be smaller.