Correct span and non-span versions of ToArray() and ToArrayPadded() methods #8734
Conversation
Avoid allocating too large buffers (len is expressed in bytes, not in Ts). Added validation to ensure len is a multiple of SizeOf<T>() before converting to array.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
I understand from failed test that pos, len, padLeft and padRight are expressed in Ts
|
Ok, I managed to build and run the tests on my own, will try to find time today to work on it and provide a correction that passes the test. |
|
I think there is an issue with pos and len signification. Are they expressed in bytes or in Ts? |
|
Do you have any input here @petersvp? |
|
Hello. Right now I cannot test all possible scenarios - feel free to revert my commit. I merely need these for a game I'm working on to optimize allocations - didn't even checked most other defines that might have existed. EDIT 2: Looks like i created a chain of mess after me :// And I'm very sorry for this... |
|
@petersvp no worries, it's not easy to catch regressions when not found in tests. The regression went into flatbuffers because a test case/setup was and is missing for the scenario (i.e a test run with enabled ENABLE_SPAN_T and UNSAFE_BYTEBUFFER). So I think that should be adressed in any attempt to fix the regression, both to verify the fix and to avoid future regressions. @aardappel can you provide insight here about the ByteBuffer details mentioned at #8734 (comment)? |
|
@bjornharrtell @vsmcea offsets and sizes are meant to be in bytes, since this functionality is used to access different sizes of types. That said, the C# ByteBuffer was simply a clone of whatever we had in the Java implementation, to make porting easier, but it this point it can do whatever you want it to do. But universally working with bytes is definitely more sane. |
|
OK I agree, so if you don't mind I will revert to my first correction that made the assumption that parameters are expressed in bytes and correct the unitary test to make it coherent with that. And add a test for ENABLE_SPAN_T and UNSAFE_BYTEBUFFER. |
|
Sounds good @vsmcea, thanks for your efforts. |
All functions parameters expressed in bytes for homogeneity Tests run: - UNSAFE_BYTEBUFFER=true/ENABLE_SPAN_T=true: passed - UNSAFE_BYTEBUFFER=true/ENABLE_SPAN_T=false: passed - UNSAFE_BYTEBUFFER=false/ENABLE_SPAN_T=false: passed - UNSAFE_BYTEBUFFER=false/ENABLE_SPAN_T=true: configuration forbidden by compilation Correction of FlatBuffers.Test.csproj to allow UNSAFE_BYTEBUFFER/ENABLE_SPAN_T tests Correction of FlatBuffersExampleTests.cs: I think the test was not run because it could not pass (to be reviewed carefully)
|
Just signed my CLA but don't kown how to relaunch the check. |
bjornharrtell
left a comment
There was a problem hiding this comment.
Looks very good to me!
|
@aardappel will you consider this for merge? |
|
Thanks @vsmcea @bjornharrtell |
Hypothesis: len, pos, padLeft and padRight are expressed in bytes.