C# endianness fixes #1964
C# endianness fixes #1964
Conversation
|
Okay this should be ready for merge |
crates/bindings-csharp/BSATN.Runtime.Tests/BSATN.Runtime.Tests.csproj
Outdated
Show resolved
Hide resolved
bfops
added
bugfix
## Description of Changes We have included the packages corresponding to clockworklabs/SpacetimeDB#1964. ## API Fixes API breakage related to endianness. ## Requires SpacetimeDB PRs SpacetimeDB branch name: release/v1.0.0-rc1-hotfix2 ## Testsuite SpacetimeDB branch name: release/v1.0.0-rc1-hotfix2 ## Testing CI only --------- Co-authored-by: Mazdak Farrokhzad <[email protected]> Co-authored-by: Zeke Foppa <[email protected]> Co-authored-by: Jeremie Pelletier <[email protected]> Co-authored-by: Ingvar Stepanyan <[email protected]> Co-authored-by: SteveGibson <[email protected]> Co-authored-by: Steve Boytsun <[email protected]> Co-authored-by: John Detter <[email protected]> Co-authored-by: John Detter <[email protected]> Co-authored-by: Tyler Cloutier <[email protected]> Co-authored-by: Tyler Cloutier <[email protected]>
| Debug.Assert( | ||
| source.Length == Marshal.SizeOf<T>(), | ||
| $"Error while reading ${typeof(T).FullName}: expected source span to be {Marshal.SizeOf<T>()} bytes long, but was {source.Length} bytes." | ||
| ); |
There was a problem hiding this comment.
Now everything gets a length check :)
There was a problem hiding this comment.
Approving with one final comment.
It probably needs its own anyway, since hex string parsing doesn't check for odd length.
I think this part got missed - passing an odd-length hex string like 012 would just ignore the 2 instead of throwing an error.
The rest looks good, thanks!
| Assert.NotNull(addr); | ||
| Assert.Equal(addr.ToString(), str); | ||
|
|
||
| byte[] bytes = |
There was a problem hiding this comment.
If you want, you can use Convert.FromHexString here.
We can't use it in BSATN.Runtime because it's not part of .NET Standard which we need for Unity, but no harm in using it in tests.
There was a problem hiding this comment.
I see you used it in the identity roundtrips tests below, but not in this function. Seems unintentional?
kazimuth
force-pushed
the
jgilles/csharp-endianness
branch
from
28c430a
to
882bb54
Compare
|
I'm nervous about these internal test failures, I don't know what's causing them. |
These are a known issue that should hopefully be fixed by tomorrow. |
| var writer = new BinaryWriter(memoryStream); | ||
| var bsatn = new Identity.BSATN(); | ||
| bsatn.Write(writer, ident); | ||
| writer.Flush(); |
There was a problem hiding this comment.
Same nit re: using (...) block, like you do in address now.
Description of Changes
Similar fixes to the previous endianness fixes (#1926) for C#.
API and ABI breaking changes
Reverts a previous API break which (I believe) printed Identities and Addresses backwards in C#. Otherwise, only documents existing behavior & adds methods.
Expected complexity level and risk
1
Testing
I am not sure how to add unit tests to the C# bindings, @RReverser please advise. E: NVM, figured it out.