mobiusklein commented on code in PR #251:
URL: https://github.com/apache/arrow-dotnet/pull/251#discussion_r2752257564
##########
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 went through and factored the expression three different ways.
1. `(length) * (bitWidth / 8)`
2. `(length / 8) * bitWidth`
3. `(int)((ulong)length * (ulong)bitWidth / 8ul)`
They all fail if the final product exceeds 2 ** 31 - 1, but the only the 3rd
option makes it abundantly clear that the code is attempting to knowingly
manipulate values whose domain may exceed it. They are all equivalent within
the scope of the `checked` expression since the error message won't tell you
where in the expression tree the overflow happened.
Suffice to say, this whole conversation is why I used `ulong`s, assuming
the abstracted logic was correct and that its constrained implementation on
.NET's hamstrung addressing integer type system was the problem.
Outside of a very few specialist embedded ASICs or historical hardware, a
byte is always 8 bits. Furthermore, it's _required_ by the language that a
`byte` be 8 bits:
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/integral-numeric-types.
If we were running .NET code on hardware where that rule didn't hold, I think
the something else would either already be emulating the 8 bit byte or else
have halted and caught fire already. I don't think the Arrow C++ library even
has the provision for non-8-bit bytes since you'd need byte offsets to work
with string arrays of any sort.
--
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]
