On 9/6/11 2:53 PM, Jonathan M Davis wrote:
Okay. Onto the next item in the review queue. I'm going to be the review
manager this time around, and we're going to be reviewing dsimcha's region
allocator. The review starts now and will end on 2011-09-21 at midnight in UTC
(so about 2 weeks from now, when the 21st begins). Assuming that we're ready
to vote at that point, I'll start a thread for voting for inclusion in Phobos,
and the vote will last a week. If it's not ready to be voted on for some
reason (such as dsimcha needing to go back and make a bunch of changes that
are going to need to be reviewed), then voting will be postponed and it'll be
put back in the review queue once it's ready for review again.
Docs:
http://cis.jhu.edu/~dsimcha/d/phobos/std_regionallocator.html
Unfortunately my review is incomplete because I don't see the allocator
interface.
In theory it should be possible to first define the region allocator and
then the allocator interface. The other way around is also valid.
However, clearly it's best to define the general allocator interface
_together_ with the region allocator.
Containers will need an allocator, and I think it's best to make it a
straight dynamic interface. In that design, the region allocator would
offer the scoped structs PLUS factory methods in those structs that
return implementations for those interfaces.
* It's region_allocator or region.allocator, not regionallocator.
Ironically a good example of why is in this very name - is it "region
allocator" or "regional locator"?
* Use "bumped up" and "bumped down" or something instead of
"incremented" and "decremented", which imply 1 as the unit.
* Enumerated paragraphs are not visually distinguished.
* Synopsis should go before the textual intro.
* Exception should be at the bottom, not the top of the dox.
* There's a problem with "std.regionallocator
RegionAllocator.regionallocator RegionAllocator" - i.e. a macro that
didn't expand to what it should have.
* I didn't understand from the dox what the relationship between
RegionAllocator and RegionAllocatorStack is, mainly because
RegionAllocator is used before having been defined. (This is a difficult
problem in general.)
* The GCScan enum is defined uncomfortably far from the places where
it's relevant. (Another difficult matter.)
* I think "scanThreadLocalStack" should be "threadLocalStackIsScannable"
so as people don't confuse it with an action.
* newArray could only take the element type as a template parameter; the
number of dimensions in unequivocally determined from the number of
arguments.
* In fact new(Uninitialized)Array should not be a member as its workings
are not specific to this allocator. It should be a free function taking
an allocator as a parameter.
* The documentation for resize() is the first time "allocator interface"
is mentioned (without having been defined). Wait a minute. I was
hungrily looking for this since I opened the doc.
* I think alignBytes should be a compile-time computable property, and
that the documentation should mention that.
* isAutomatic, isScoped, freeIsChecked should also be static and
statically computable properties (perhaps not freeIsChecked).
Andrei
