chutchinson commented on a change in pull request #7158:
URL: https://github.com/apache/arrow/pull/7158#discussion_r437589084
##########
File path: csharp/src/Apache.Arrow/ArrowBuffer.Builder.cs
##########
@@ -80,27 +140,59 @@ public Builder<T> AppendRange(IEnumerable<T> values)
return this;
}
- public Builder<T> Reserve(int capacity)
+ /// <summary>
+ /// Reserve a given number of items' additional capacity.
+ /// </summary>
+ /// <param name="additionalCapacity">Number of items of required
additional capacity.</param>
+ /// <returns>Returns the builder (for fluent-style
composition).</returns>
+ public Builder<T> Reserve(int additionalCapacity)
{
- EnsureCapacity(capacity);
+ if (additionalCapacity < 0)
+ {
+ throw new
ArgumentOutOfRangeException(nameof(additionalCapacity));
+ }
+
+ EnsureAdditionalCapacity(additionalCapacity);
return this;
}
+ /// <summary>
+ /// Resize the buffer to a given size.
+ /// </summary>
+ /// <remarks>
+ /// Note that if the required capacity is smaller than the current
length of the populated buffer so far,
+ /// the buffer will be truncated and items at the end of the
buffer will be lost.
+ /// </remarks>
+ /// <remarks>
+ /// Note also that a negative capacity will result in the buffer
being resized to zero.
+ /// </remarks>
+ /// <param name="capacity">Number of items of required
capacity.</param>
+ /// <returns>Returns the builder (for fluent-style
composition).</returns>
public Builder<T> Resize(int capacity)
{
+ capacity = capacity < 0 ? 0 : capacity;
Review comment:
It's hard to say why I chose this behavior; this was written over a year
ago. I can only imagine my thought process was that the capacity "can't go any
lower", and avoiding the costs that come with throwing an exception.
The unit test existed to verify the behavior as written, not necessarily to
satisfy requirements, which I would agree was misguided.
My current stance would be to match the behavior of the more mature
implementations, and throw **ArgumentOutOfRangeException**. I tend to prefer
avoiding exceptions and bounds checking where possible, but as a consumer I
could understand being surprised to find that passing a negative capacity
results in an empty buffer. I'm not sure I would expect this method in a
situation where the bounds checking or exception stack unwinding would be
harmful enough to be undesirable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
