Thanks for taking the time to do this review. I appreciate the effort and the constructive criticism.
== Quote from Christophe (trav...@phare.normalesup.org)'s article > Concerning the gcallocator API, why is there no template method that > forwards to new T? Is the user obliged to call allocate, and then > initialise the object manually? Several people have asked this and I think it needs to be added. It's obviously trivial to implement, but I'm not really sure what to call it yet. > Would it not be useful if all Allocators use Exceptions that derives > from the same Exception subtype ? Hmm, this might get messy in terms of catching other exceptions when forwarding to e.g. std.array.array() and having to throw a new type of exception. It would be nice but I'm skeptical about whether it's worth it. > Now about RegionAllocator... I took me a lot of time to understand how > it works, and I am not 100% sure about how it works. I had to rewrite my > review when I understood something new and discovered what I had > understood before was false. > This is how I think it works now: Several copies of a RegionAllocator > works just the same. When the last is destroyed, all the memory > allocated is cleared. You can build a RegionAllocator that uses the same > stack as the previous RegionAllocator. This "pauses" all the copies of > the previous allocator: they can no longer allocate or free memory until > all the copies of the new RegionAllocator are destroyed. Right. > Is this "pause" behavior really a desirable functionnality? This makes > the whole behavior of RegionAllocator very confusing, because you have > to make the disctinction between copies of a RegionAllocator and > RegionAllocator that shares the same Stack. Well... copies of a > RegionAllocator do share the same Stack, so it is confusing. It is a little confusing but it has to be this way for efficiency. If every call to newRegionAllocator() had to allocate a new block from the heap, this would largely defeat the purpose in cases where you need, e.g., a temporary array or two. > Moreover, it is rather dangerous to use. What if a copy of a > RegionAllocator sharing the same stack as a previous RegionAllocator is > kept unexpectedly alive somewhere while the previous RegionAllocator > goes back into action? A RegionAllocatorError is thrown. This gives you a decent diagnostic about what went wrong. This is checked for and you won't get stuck spending an entire afternoon debugging something silly like this. > What happens if all copies of a RegionAllocator > dies when a RegionAllocator using the same Stack and created later is > still alive? A RegionAllocatorError **should** be thrown here too, though I might have forgotten about this case. > Those allocators are supposed to be put in containers or things like > that, aren't they ? They might really have a life of their own (I would > rather have them being classes that you eventually make scope classes, > but that is not the main point). This behavior is IMO too dangerous to > be desirable in the standard library, especially if users just start to > make newRegionAllocator and do not realize they prevent each other from > working fine. I see your point but this is a fundamental limitation of the design and is absolutely necessary for efficiency. If you want to prevent this, give each container a RegionAllocator with its own RegionAllocatorStack. > If I admit that the behavior of RegionAllocator sharing the same stack > but are not direct copies of each other is really what expert users > want, this is badly documented: > - regionAllocatorStack.newRegionAllocator does not tell it invalidates > the use of previously created newRegionAllocators. I thought this was clear from the documentation of the RegionAllocator struct, though it may be important enough to beat people over the head with a little more instead of just saying it once or twice. > - Same for .newRegionAllocator. Moreover, the name of this function is > very confusing. One could think it is brand new, whereas it reuses the > same thread-local stack. > - Introduction of RegionAllocator comment do not tell that copies of a > RegionAllocator are different from RegionAllocators sharing the same > stack (but that do not share the same correctRegionIndex). Again, the semantics are unavoidably somewhat complicated and delicate. The only good way I could think of to document them was through examples. > - regionAllocator.allocate do not tell it checks it is the lastly > created RegionAllocator that shares the same stack (but not the same > correctRegionIndex). correctRegionIndex is just a unique identifier for each RegionAllocator instance within each stack, and is an implementation detail. I don't understand the problem here. > Now, do the users of RegionAllocator really want memory to be > automatically freed when the RegionAllocator is destructed with no > explicit request, and no way to prevent this? This is the whole point of scoped allocators. I can't think of a less dangerous way of implementing something like this. If you want to return RegionAllocator-allocated memory, take a RegionAllocator as a parameter instead of creating one. > Important point: > opAssigns seems not to support self assignment of a unique copy of a > RegionAllocator. This is a bug that needs to be fixed. Thanks for reporting it. > I am really sorry if I sound too agressive, and make it sound like all > these functionality are not useful. This RegionAllocator may have > applications and advantages that I am not aware of. I just tought when I > started to read the documentation and the code that this allocator would > bring to the standard library an easy and rather safe way to allocate > memory on a stack by moving a pointer up and down when memory is > requested or discarded, whereas this allocator is much more complicated > than that. This review contains nothing but perfectly reasonable, constructive criticism. I appreciate it. My only concern is that I can't think of how to simplify RegionAllocator without making it less powerful, and since it's an advanced feature anyhow, I think power needs to be more important than simplicity.
