Add Variant serdeProxy support #24
Conversation
support e.g. deserializing int to Variant!(void, ProxiedInt) Side-effect: this allows us to easily implement serdeProxyCast, which is done here and supersedes the PR libmir#22
WebFreak001
force-pushed
the
add-variant-proxy-support
branch
from
4e62d25 to
46b75e5
Compare
| return to!To(v); | ||
| } | ||
|
|
||
| To proxyTo(To, From)(auto ref scope return From v) |
There was a problem hiding this comment.
Please do not introduce a separate function for that logic.
There was a problem hiding this comment.
Instead, place the logic where it is needed.
There was a problem hiding this comment.
would it be ok to put them in the ser/deser packages? the same code is reused multiple times, that's why I made functions for the a little more complicated/configurable assignment logic.
Having that in a function means there is just a single place to change to control how proxies are assigned.
| @@ -562,3 +562,35 @@ version(mir_ion_test) unittest | |||
| ]) | |||
| text.text2ion.ion2msgpack.msgpack2ion.ion2text.should == text; | |||
| } | |||
|
|
|||
| To toProxy(To, From)(auto ref scope return From v) | |||
There was a problem hiding this comment.
Ditto, place the logic where it is needed.
| @@ -442,6 +443,7 @@ template deserializeValue(string[] symbolTable) | |||
| if (!isFirstOrderSerdeType!T) | |||
| {with(params){ | |||
| import mir.algebraic: isVariant, isNullable; | |||
| import mir.serde: ContainsProxied; | |||
There was a problem hiding this comment.
Why not Contains!(staticMap!(serdeGetFinalProxy, Types))?
There was a problem hiding this comment.
good idea, didn't think of that, but probably a lot simpler
| || contains!int | ||
| || contains!uint | ||
| || contains!long | ||
| || contains!ulong; |
There was a problem hiding this comment.
That should be a separate MR with tests.
| alias contains = Contains!Types; | ||
| alias contains = ContainsProxied!Types; | ||
|
|
||
| pragma(inline, true) void setValue(T)(T new_value) |
There was a problem hiding this comment.
That shouldn't be a common logic. What is your use case?
There was a problem hiding this comment.
this function is only within the isVariant branch, it's the logic to set a variant to a value that might be using a proxy to a proxy-supported value.
This function is the core functionality of this PR and makes the added example pass.
There was a problem hiding this comment.
It is a too advanced feature for the Dlang's small community. It may make other features like better @nogc support via @serdeScoped harder to implement. Please describe what is your use case. I wonder if we can find another way to support it.
There was a problem hiding this comment.
do you mean the problem is that this is a local function with scope? I can also make it static and use a ref parameter to take in the value to change.
The function body should have no problem with @nogc if using @serdeProxyCast. The branch that may potentially allocate is also only compiled in if there is a variant that has types which are proxies to primitives / otherwise unsupported value types of the variant.
There was a problem hiding this comment.
No. I mean that there is unlikely should be any function of any kind. The idea itself is good. However, it may block some other code evolution paths. So, could we replace it with minimal hardcoded if else conditions for a minimal amount of types? What types do you need to support?
|
closing this for now as this tries to cover too many use-cases. For me right now I only really need #22, which I wanted to generalize here. Originally the idea was using this PR, #22 can be simulated by using a struct with serdeProxy inside a variant and the following support code: template MyEnumProxyImpl(T)
{
@serdeProxy!T
struct MyEnumProxyImpl
{
T value;
alias value this;
this(T value)
{
this.value = value;
}
}
}
alias serdeEnumProxy(T) = serdeProxy!(MyEnumProxyImpl!T); |
support e.g. deserializing int to Variant!(void, ProxiedInt)
Side-effect: this allows us to easily implement serdeProxyCast, which is
done here and supersedes the PR #22
see test case to see what it actually fixes.
Depends on libmir/mir-algorithm#420