Hotfix conversion of float/double #49
Conversation
There was a problem hiding this comment.
The assumption that the runtime would treat Action similarly to Action is apparently invalid
Because for value types specialized code for the generics is created, with each optimized. Thus there may be differences.
Also on sharplab you can choose between x64, x86 and the old .NET Fx.
| @@ -3,7 +3,7 @@ | |||
|
|
|||
| namespace Sally7.ValueConversion | |||
| { | |||
| public delegate int ConvertToS7<TValue>(TValue? value, int length, Span<byte> output); | |||
| public delegate int ConvertToS7<TValue>(in TValue? value, int length, Span<byte> output); | |||
There was a problem hiding this comment.
Is the in actually needed?
For small TValue (roughly 2x CPU register-size) this is actually a de-optimization, as the value has to be passed via stack rather than cpu's registers (as w/o in).
For value types the generic constraint : struct would guarantee the readonly-ness here too (copy by value).
If arrays need to be passed in, then I'd use ReadOnlySpan instead.
There was a problem hiding this comment.
Depends on what you'd call needed. I could add separate float/double handling (under the assumption that'll work as expected in that case), but apparently there's a difference in how the framework treats floats vs ints, while I would expect that it would just pass 4 bytes around in the same way. The same also applies for double vs long.
So maybe it's a good idea to split float / double regardless since it removes the downsides, but it was definitely broken in the previous state.
There was a problem hiding this comment.
For .NET 6+ I'd throw away the delegate and use an approach that is based on generic math (i.e. static abstract interface members).
- no problems with endianess (I believe that's the root issue here)
- superb perf (delegates aren't as optimized as struct's generic specialization)
- easy to extend
When we have some time with this I can look at the weekend into that approach.
There was a problem hiding this comment.
Your help is always greatly appreciated! I think main problem here is that the conversion also supports strings and arrays, so I'm not sure we can handle all in a uniform fashion. But perhaps it's a good idea to differentiate anyway. I must admit I haven't applied static abstract interface members anywhere yet, even though I was hugely looking forward to its implementation before .NET 6. Guess I've been too busy lately 😉
There was a problem hiding this comment.
OK, when the test pass here let's go ahead and merge this.
I'll create an issue that reminds me on working with the static abstract interfaces for this. For strings and arrays I don't see a problem -- when there's a PR for this I hope you see why there shouldn't be a problem.
--> #50
There was a problem hiding this comment.
I actually already released this as 0.14.4. The hotfix branch doesn't cleanly merge into main though, because I did some work on main that's not yet released. Like I said, been a bit too busy, will try to prepare a release of main sooner rather than later.
| @@ -48,14 +48,14 @@ public static Delegate GetConverter<TValue>() | |||
| throw new NotImplementedException(); | |||
| } | |||
|
|
|||
| private static int ConvertFromLong(long value, int length, Span<byte> output) | |||
| private static int ConvertFromLong(in long value, int length, Span<byte> output) | |||
There was a problem hiding this comment.
Same (and on other places).
There was a problem hiding this comment.
Yeah these all follow ConvertToS7<TValue>, so they align with the delegate definition.
The reference is required to access the data as byte array, but due to converter changes is now available as `in` only.
Fix conversion of single
float/doublevalues to their PLC format. The assumption that the runtime would treatAction<float>similarly toAction<int>is apparently invalid, as can also be witnessed from SharpLab.