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 14:16:38

Stewart, Robert skrev:
> Thorsten Ottosen wrote
> On Monday, May 18, 2009 9:05 AM
>> Dmitry Vinogradov skrev:
>>
>>> Does any container exist to offer functionality like
>> Boost.Array but
>>> allowing to store from 0 to N elements? Is there any
>> interest in such
>>> container?
>>>
>>> PS. It's similar to fixed_string but as a generic container.
>> Please see
>>
>> http://www.cs.aau.dk/~nesotto/boost/auto_buffer.zip
>>
>> http://www.cs.aau.dk/~nesotto/boost/trunk/libs/auto_buffer/doc/html/
> 
> auto_buffer looks really useful, Thorsten, but I'd like to pick at it a little.

That's alright :-)

> boost::default_grow_policy is troubling.  First, s/grow/growth/ in the name.  Did you mean to that policy as appropriate for all of Boost? 

> You put it in the boost namespace.  

I put everything in the boost namespace for now. But if people would 
like it to be in boost::auto_buffers, I can put it there. But I would 
like that answered in the review.

>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.

> 
> 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.

>The "GrowPolicy" template parameter should be named "GrowthPolicy."

Really?

> 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.

> Is there a compile time assertion for N > 0 for store_n_objects?  

No, you are allowed to specify an empty stack buffer.

> 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).

> There should be a debug build assertion that GrowthPolicy::reserve(n) returns a value >= n to help diagnose mistakes.

Yup. Not there yet, but I'll add it.

> "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?

> What's the point of push_back() with no argument?  

>What's the motivation for that versus making the caller explicitly default construct a T and pass that to push_back(by_value_type)? 

> Are you simply optimizing the copying?  If so, isn't it better to mimic vector and gain the optimization with move semantics?

I guess it should be rename emplace(), like in vector.

> 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.

> 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.

> The remarks for the uninitialized_*() functions use the phrase, "depending on the application," 

>but that doesn't explain when the user is responsible to initialize or destroy the n elements.  

>Change the remarks to note that if T is POD, the user need not initialize/destroy the affected elements, whereas not doing so for non-POD types results in undefined behavior.

Your explanation is better, thanks.

> 
> The unchecked_*() functions deserve remarks that explain the danger in their use and that the precondition is not checked. 

> If you don't already, though, you should check the precondition in debug builds to help clients use these dangerous functions correctly.

Precondition is checked in debug builds.

> The description of should_shrink(), on the default_grow_policy class, mentions shrink_to_fit() which is not mentioned 

>elsewhere in the documentation.  I find shrink_to_fit() in Boost.Interprocess, but nowhere else.  Did you forget something?

The docs are not a 100% done. The function is implemented though.

> shrink_to_stack() might be a useful addition.  

>You probably inferred the purpose: to shrink the size to what will fit on the stack and move the contents to the stack from allocator-supplied memory, if used.  

>That seems the logical converse of dynamically growing beyond the stack allocation, though I admit I can't think of a

> motivating use case beyond simply trying to keep memory consumption low in a long running function.

shrink_to_fit() can already place the content on 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 :-)

-Thorsten