Re: [boost] [auto_buffer] Interest check: container with preallocated buffer

Subject: Re: [boost] [auto_buffer] Interest check: container with preallocated buffer
From: Thorsten Ottosen (thorsten.ottosen_at_[hidden])
Date: 2009-05-19 16:41:28

Stewart, Robert skrev:
> Thorsten Ottosen wrote
> On Tuesday, May 19, 2009 2:17 PM
>> Stewart, Robert skrev:
>>> Third, new_capacity() ought to be "reserve" because that
>> more clearly associates it with auto_buffer<>::reserve() and
>> the well understood behavior of, for example,
>> std::vector<>::reserve().
>>
>> Well, the function of the policy does not reserve anything itself, it
>> just calculates the new capacity. Hence the name.
> 
> I understand your thinking.  How about "grow?"  It is suggestive of the purpose but less wordy.

Any verb suggests a modification is taking place IMO. I think the 
function is named quite ok, but let the review decide it.

> Is the signature actually sufficient?  What if the requested size exceeds the adjustment new_capacity() would make?  

>IOW, what if the requested size is greater than four times the old capacity?  Would this be better?
> 
>    template <class Size>
>    static Size
>    grow(Size _old, Size _size);
> 
> This means supplying the old capacity and desired size such that the result should be at least _size.

In that case the exact requested size is allocated. The interface you 
show provides some extra flexibility, but I don't know if it is useful. 
Perhaps.

>>> auto_buffer's "StackBufferPolicy" template parameter should
>>> be named "SizingPolicy" because it has to do with computing
>>> the buffer's size.
>> But it doesn't has much to do with the size of the container
>> in general.
>> So that name does not sound too good either.
> 
> That's why I didn't suggest "SizePolicy."  Seriously.  The policy still has to do with a size computation.  "StackSizePolicy" is probably better.

StackSizePolicy is fine with me.

>>> The "GrowPolicy" template parameter should be named "GrowthPolicy."
>> Really?
> 
> Yes, "growth" is a noun.

How about ResizingPolicy? The policy is also used in shrink_to_fit().

>>> The auto_buffer nested value "N" is confusing as it will
>>> change based upon the SizingPolicy,
>>> but won't match that policy's "value" if using
>>> boost::store_n_bytes, and it isn't the number of elements in,
>>> or capacity of, the auto_buffer, particularly when memory is
>>> allocated from the allocator.
>>> Renaming it to "stack_capacity" should work, though.
>> I'll think about it. I need a short name for the docs, and N
>> seems both
>> short and good to me. N is also documented.
> 
> "N" suggests number of elements to me.

Right. And that is what it is.

>>> Is there a compile time assertion for N > 0 for store_n_objects?
>> No, you are allowed to specify an empty stack buffer.
> 
> Why would anyone bother to create one that won't fit on the stack?  Why not just use vector?  

Because auto_buffer provides additional functionality not found in 
std::vector.

>Are you thinking of generic code that sometimes will succeed in using the stack and other times not?  

I don't know what you mean by this, but I'm happy if there is yet 
another reason to use the class.

>I suppose that's a good justification, but it would be worth documenting that N == 0 is permissible and why.

yes, it should be documented.

>>> How about that store_n_bytes<N>::value >= sizeof(T)?  Those
>>> will help to diagnose misuse.
>> This might be ok to have if  0 < N < sizeof(T).
> 
> I suppose this can occur for the same reason as above but it should be documented.  Otherwise, there's no sense in allowing it.

OTOH, I would hate to put a static assertion in there if it sometimes
complicates the use when the user actually don't mind this.

>>> "optimized_const_reference" would be better named
>>> "by_value_type" or something.
>>> For small types, optimized_const_reference isn't a reference
>>> at all, so the name is misleading.
>>> Better to name it for the usage rather than the type.
>> But then for large types, "by_value_type" is not a value type, but a
>> reference. What about parameter_type or param_type?
> 
> Yeah, it doesn't work well for the other case.  "Parameter" suggests only the input side of things.  boost::call_traits does something similar, but that only has to do with calling functions.  It would be odd to return a call_traits type.  How about "fast_value_type?"

It's a candidate, but again, sometimes it is a reference :-)

>>> Shouldn't pop_back_n() be an algorithm rather than a member
>>> function?
>>> Is there really efficiency to be gained by making it a
>>> member and is there a good use case for that?
>> Yes, it is more efficient since for PODs it only adjust the size.
>> Calling erase() would be slightly less efficient.
> 
> The documentation should note that the function is particularly efficient for PODs.

yup.

>>> Rather than reserve_precisely(), which should be named
>>> "reserve_exactly" if retained,
>>> why not just document reserve() as allocating what
>>> GrowthPolicy::reserve() returns and leave the behavior to the
>>> policy?
>>> That leaves room for reserve() allocating exactly what's
>>> requested, or rounding up according to some computation, as
>>> the policy dictates.
>> reserve() is used several places when the buffer needs to
>> grow. If this
>> was to allocate precisely what was requested, then we would e.g.
>> allocate just one extra element.
>>
>> But I guess I could factor the code so it was as you
>> suggested. I will
>> make this a review question. This will require a small change in
>> GrowPolicy so you can see what is requested.
> 
> I think the "see what is requested" is the change I suggested above.

Not quite. reserve_precisely() allows you to do extractly that, not 
matter what GrowPolicy you have installed. The user knows that the
new buffer should be exactly what he asks for, no matter what the
policy thinks.

>>> The unchecked_*() functions deserve remarks that explain
>>> the danger in their use and that the precondition is not checked.
> 
> You didn't comment on the addition of remarks for these functions, so I'm re-raising this point.

The precondition is checked with BOOST_ASSERT(). As for remarks, yeah, 
but how many other functions in Boost have a remark saying that you 
should not violate the preconditions?

>>> shrink_to_stack() might be a useful addition.
>> shrink_to_fit() can already place the content on the stack.
> 
> OK.  I assume that shrink_to_fit() takes an argument, so that shrink_to_stack() would be shorthand for a.shrink_to_fit(a.stack_capacity).

No, shrink_to_fit() never erases elements from the container 
(logically). resize() does, but this is on my todo list. resize() would 
probably not move elements to the stack.

>>> There are numerous things to fix in the docs, plus more to
>>> add, of course, but I don't want to address those points now.
>>> I think I can find some time to help you flesh out the docs
>>> so you can submit auto_buffer for review, if you're interested.
>> It's already in the review queue. But I need a review manager :-)
> 
> I guess you'd rather I chose to be review manager than to help on the docs.  I'll do it then.

Well, I don't mind both :-) But the sooner I have a review manager, the 
sooner we can get the review. Thanks!

-Thorsten