On 6/16/2011 11:46 PM, Andrei Alexandrescu wrote:
On 6/11/11 11:26 AM, dsimcha wrote:
I've overhauled my TempAlloc proposal based on some of the suggestions
I've received.
Review:
Thank you for the review.
* Mention no safety at disadvantages: deallocating memory may leave
pointers dangling.
Good point.
* The synopsis should use scope(exit) TempAlloc.frameFree(); as that is
a robust idiom.
Good point.
* newArray -> "For performance reasons, the returned array is not
initialized." Then call it newUninitializedArray. Make safety the
default and lack thereof the verbose alternative.
Here I'm going to have to strongly disagree with you. The whole point
of TempAlloc is that it's a performance hack. In general I agree it's
better to do the safe thing by default, but libraries specifically
written as performance hacks are an exception. In TempAlloc it makes
sense to do the efficient thing by default. I personally would never
use newArray as opposed to newUninitializedArray and
TempAlloc.newUninitializedArray (31 characters) feels **ridiculously**
verbose and would clutter the API.
* newArray -> "It is not scanned for pointers by the garbage collector
unless GC.addRange is called." I'd agree that malloc() shouldn't do so
because it doesn't know the type, but with newArray you do know the type
so you could automatically call GC.addRange appropriately. If the client
doesn't want that, they can obtain it with extra syntax. Since now you
have two things (uninitialized and noscan) you may want to make them
function parameters instead of part of the function name.
Again, I'm going to have to strongly disagree with you here. Adding the
range to the GC would require taking the GC lock, which is to be avoided
like the plague. In a library that exists specifically as a performance
hack, things like this should be explicit if there's any doubt about the
user's intent. This should come before safety. Once you require this
explicitness, there's no need for the TempAlloc API to handle it
anymore, because the user can just call GC.addRange.
Furthermore, the GC's management of ranges isn't very efficient (I think
it's O(N)) so if you have a whole bunch of TempAlloc-allocated arrays,
things are going to grind to a halt. Lastly, extra bookkeeping would be
necessary to decide whether to call removeRange().
* I'm a bit unsure about free(). People already have the pointer, so
free() should use it as a cheap way to check that they're freeing the
right thing. There should be free(void*), freeLastAllocated(), and
possibly destroyAndFree(T)(T *).
Probably a good idea. So free(void*) would just do a check and then
call freeLastAllocated()? I'm not sure about destroyAndFree(). What
semantics are you suggesting for it? Calling the d'tor if any and then
freeing?
* segmentSlack() -> segmentAvailable().
I guess that's a little more clear.
* stackCat() is a bit surprising. A better way would be something like a
TempAlloc appender that accepts a chain().
* In fact looking at tempArray() just below that's all - it can accept
chain(). No need for stackCat.
Good point. I kind of didn't like stackCat anyhow.
* mixin newFrame should be renamed to scopedFrame.
Hmm. I kind of like newFrame. Why is scopedFrame a better name? If I
understood why it's better, I'd be more inclined to change it.
