Stewart, Robert skrev:
Thorsten Ottosen wrote On Tuesday, May 19, 2009 2:17 PM
Third, new_capacity() ought to be "reserve" because that more clearly associates it with auto_buffer<>::reserve() and
Stewart, Robert skrev: 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