> Right. OK, I'm glad I got the way RegionAllcator works right.
dsimcha , dans le message (digitalmars.D:144124), a écrit : >> 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. Why catching other exceptions? This could apply to exceptions thrown by the allocator itself only. If std.array.array() throws, it is not the problem of RegionAllocator. But it was a minor comment. > 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. Having a RegionAllocator with no shared stack does not prevent you from using a default thread-local RegionAllocator that you use for a temporary array or two. You're implementation adds the functionality to throw if two instances of the same RegionAllocator with different correctRegionIndex are competing, which I agree is a good thing for debuging. >> 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. ;) >> - 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. There is nothing wrong with not telling about correctRegionIndex, but there are 2 problems: - allocate tells nothing about checking for the existence of a competing RegionAllocator and eventually throwing, but it does check and throw (as it should). - even if you told you checked for the existence of a competing allocator, like you did with free, talking about a possible RegionAllocator that shares the same RegionAllocatorStack is confusing, because a direct copy of a RegionAllocator is also an instance of RegionAllocator that shares the same RegionAllocatorStack, but fortunately do not induce exception throwing. This needs clarification. > 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. OK, there could be another kind of RegionAllocator that is not a scoped allocator, but it would not be as powerful as a scoped RegionAllocator. >> 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. ;) > 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. Ok, don't drop this feature then. But the API is too complicated. I think you can keep the same functionality with an API that is much easier to understand: - Make RegionAllocatorStack private. This is implementation detail no one has to know about. - Create a method for RegionAllocator that is called something like "invalidatingCopy", which is the same as creating a new regionAllocator with the same regionAllocatorStack. Tell about the fact that the invalidatingCopy make the calls to the RegionAllocator being copied throw until all instances of the invalidatingCopy go out of scope. - Make a function to call "invalidatingCopy" on the default thread-local RegionAllocator, to replace newRegionAllocator, but I insist, please use a different name since it is confusing: it makes you think the allocator is breand new, whereas it reuses a common stack. - Make a function to create a brand new allocator with its own stack, and specifies that the previous function may be more efficient. With this, I think everybody will understand easily what is an invalidatingCopy of your allocator. You can explain in allocate, free and other methods that you check for the existence of an invalidatingCopy without having to tell anything about sharing RegionAllocatorStack with other RegionAllocators. I hope this helps. Regards. -- Christophe
