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

Subject: Re: [boost] [auto_buffer] Interest check: container with preallocated buffer
From: Stewart, Robert (Robert.Stewart_at_[hidden])
Date: 2009-05-19 15:02:04

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.

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.

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

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

Yes, "growth" is a noun.

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

> > 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?  Are you thinking of generic code that sometimes will succeed in using the stack and other times not?  I suppose that's a good justification, but it would be worth documenting that N == 0 is permissible and why.

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

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

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

That name strikes me as odd, but it will become the norm, so yes, you should use it.

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

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

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

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

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

_____
Rob Stewart                           robert.stewart_at_[hidden]
Software Engineer, Core Software      using std::disclaimer;
Susquehanna International Group, LLP  http://www.sig.com

IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses.