C#: Allow passing variable number of args to command. #1208
Conversation
|
This PR causes a slight performance degradation - below 10%, but I didn't conduct enough benchmarks in order to be precise. |
| impl From<::protobuf::EnumOrUnknown<ProtobufRequestType>> for RequestType { | ||
| fn from(value: ::protobuf::EnumOrUnknown<ProtobufRequestType>) -> Self { | ||
| match value.enum_value_or(ProtobufRequestType::InvalidRequest) { | ||
| ProtobufRequestType::InvalidRequest => RequestType::InvalidRequest, |
There was a problem hiding this comment.
Is is possible to use a macro?
There was a problem hiding this comment.
dunno, you're welcome to try :)
|
|
||
| public async Task<string?> SetAsync(string key, string value) | ||
| { | ||
| var args = this.arrayPool.Rent(2); |
There was a problem hiding this comment.
Please avoid this. references to avoid linter warnings in future
There was a problem hiding this comment.
I'm keeping with the current style. You're welcome to change the style & add a linter to enforce it.
There was a problem hiding this comment.
As a side note, my pr for enforcing rules #1136 (in particular this one) still waiting for approval. So it might be beneficial to merge it first
| RequestType::Select => Some(cmd("SELECT")), | ||
| RequestType::ConfigGet => Some(get_two_word_command("CONFIG", "GET")), |
There was a problem hiding this comment.
Is it possible to use a macro (2 macros) here?
There was a problem hiding this comment.
maybe. Your suggestion is unclear, and I'm not taking an open-ended task now.
| GCHandle pinnedArray = GCHandle.Alloc(args, GCHandleType.Pinned); | ||
| IntPtr pointer = pinnedArray.AddrOfPinnedObject(); |
There was a problem hiding this comment.
Won't stackalloc be faster?
There was a problem hiding this comment.
dunno, you're welcome to profile :)
There was a problem hiding this comment.
@Yury-Fridlyand can you open an issue to check if later on when we'll continue C# develop?
This will allow us to expand the C# API to support all commands.
Hi, I'm going on vacation and will spend a minimal amount of time getting this PR merged. I won't have time for open-ended "maybe do " comments - if you see concrete problems, please point them out. Anything else can be done by someone else, later. |
* Expose request type enum which is independent from Protobuf. * Simplify C# tests by moving shared logic to function. * C#: Allow passing variable number of args to command.
* Expose request type enum which is independent from Protobuf. * Simplify C# tests by moving shared logic to function. * C#: Allow passing variable number of args to command.
This will allow us to expand the C# API to support all commands. This is in preparation of supporting additional commands.
The core change is that Message and the FFI functions aren't written around 2 raw pointers, but they now take an array of raw pointers, which is pinned by the client until operations complete.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.