adamreeve commented on code in PR #251:
URL: https://github.com/apache/arrow-dotnet/pull/251#discussion_r2752376460
##########
src/Apache.Arrow/C/CArrowArrayImporter.cs:
##########
@@ -470,7 +470,7 @@ private ArrowBuffer[] ImportFixedWidthBuffers(CArrowArray*
cArray, int bitWidth)
int length = checked((int)cArray->offset +
(int)cArray->length);
int valuesLength;
if (bitWidth >= 8)
- valuesLength = checked(length * bitWidth / 8);
+ valuesLength = checked((int)((ulong)length *
(ulong)bitWidth / 8ul));
Review Comment:
I don't mind too much which form of expression is used here, as you say this
shouldn't be particularly performance sensitive.
I'm just concerned that there's a hidden assumption that isn't being
validated and isn't obvious when first reading this piece of code. That was a
problem with the original code but it would be nice to fix that if we're
touching it now.
From looking a bit closer, the `bitWidth` doesn't come from the C Data
Interface input but from the instance of `FixedWidthType` being used, so we
don't need to worry about invalid input and this should always be a multiple of
the byte size. So maybe just adding a debug assertion here would help make the
code clearer:
```suggestion
{
Debug.Assert(bitWidth % 8 == 0);
valuesLength = checked((int)((ulong)length *
(ulong)bitWidth / 8ul));
}
```
Yes we can assume a byte will always be 8 bits, but it is possible to define
a `FixedWidthType` instance where `BitWidth % 8 != 0`, although that would be a
bit insane and terrible for performance.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
