[C#] Add support for the ping pong test, bring support for futures that return strings, i.e. types returned via pointers.#1606
Conversation
Requires supporting futures that return strings.
dicej
left a comment
There was a problem hiding this comment.
Thanks for continuing to push this forward!
| public static unsafe uint Callback(EventWaitable e, ContextTask* contextPtr) | ||
| public static unsafe int Callback(EventWaitable e, ContextTask* contextPtr) | ||
| { | ||
| Console.WriteLine("Callback"); |
There was a problem hiding this comment.
Looks like a stray debug print statement.
| if (e.SubtaskStatus.IsStarted || e.SubtaskStatus.IsReturned) | ||
| { | ||
| waitableInfoState.SetResult(e.WaitableCount); | ||
| Interop.SubtaskDrop(e.Waitable); |
There was a problem hiding this comment.
I'm surprised to see a SubtaskDrop call here for e.SubtaskStatus.IsStarted. Don't we want to wait until it reaches the e.SubtaskStatus.IsReturned state?
There was a problem hiding this comment.
Also, the line above seems like it should only apply to e.SubtaskStatus.IsReturned as well.
There was a problem hiding this comment.
Ah, yes, I misread the go equivalent, not realising it doesn't use a break statement between cases.
| } | ||
|
|
||
| // unsafe because we are using pointers. | ||
| public static unsafe int Callback<T>(EventWaitable e, Func<T> liftFunc) |
There was a problem hiding this comment.
There's a lot of code duplicated between the two overloads of Callback. Would it be practical to combine them, e.g. by having the non-generic one defer to the generic one using a dummy type of some sort?
There was a problem hiding this comment.
Turns out that we don't need the generic version at all as the lift func parameter is not used here. Was an idea that later got dropped.
| } | ||
|
|
||
| // TODO: this is not great, we want the underlying parameter, but it is passed in operands already lifted. | ||
| pub fn strip_lift(lifted_param: &String) -> String { |
There was a problem hiding this comment.
I'll admit I don't understand why this is needed or what it is doing. Could you elaborate?
There was a problem hiding this comment.
ah yes, I meant to talk about this in the description. This stems from the chat we had and the idea to use an async function for the exported function. In C# UnmanagedCallersOnly functions cannot be marked async so what I did was from the UCO function, we call to an async function so we can do the cleanups easily. Without this strip_lift function, that pattern ends up like this
[global::System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute(EntryPoint = "[async-lift]a:b/i#one-argument")]
public static unsafe int wasmExportOneArgument(int p0) {
var task = OneArgumentAsync((unchecked((uint)(p0))));
if (task.IsCompletedSuccessfully)
{
return (int)CallbackCode.Exit;
}
// TODO: Defer dropping borrowed resources until a result is returned.
ContextTask* contextTaskPtr = AsyncSupport.ContextGet();
return (int)CallbackCode.Wait | (int)(contextTaskPtr->WaitableSetHandle << 4);
}
public static async Task OneArgumentAsync(uint unchecked((uint)(p0)))
{
var cleanups = new global::System.Collections.Generic.List<global::System.Action>();
await IExportsImpl.OneArgument((unchecked((uint)(p0))));
// TODO: task_cancel.forget();
OneArgumentTaskReturn();
foreach (var cleanup in cleanups)
{
cleanup();
}
}The signature to OneArgumentAsync has the lifted parameter expression and is not valid.
There was a problem hiding this comment.
I see, thanks for explaining. Perhaps you could expand the comment to include an example of what the input and output of the regex would be, e.g. unchecked((uint)(p0))->p0.
| [global::System.Runtime.InteropServices.UnmanagedCallersOnlyAttribute(EntryPoint = "[callback]{export_name}")] | ||
| public static unsafe uint {camel_name}Callback(int eventRaw, uint waitable, uint code) | ||
| {{ | ||
| Console.WriteLine("Callback for {export_name} creating EventWaitable with code " + eventRaw + " for waitable " + waitable + " with code " + code); |
There was a problem hiding this comment.
Did you mean to leave this in here?
There was a problem hiding this comment.
Removed this and other debugs
| | Type::Char | ||
| | Type::F32 | ||
| | Type::F64 => false, | ||
| _ => true, |
There was a problem hiding this comment.
FYI, this will probably need to be refined; e.g. resource handles and other types may not need pointers either.
There was a problem hiding this comment.
Thanks, added a TODO for now.
As per the title, this PR adds more support to the C# wit-bindgen to get the ping-pong test to pass. As part of this I've:
ICancelableReadandICancelableWriteand replaced withIVTableFreeBuffermethod and replaced withcleanupactions