On Thu, Jan 12, 2012 at 11:14 PM, Oliver Kowalke <oliver.kowalke@gmx.de>wrote:
Hello Jeff,
thx for your review
How do you store the function and function arguments passed to the context constructor?
the function arguments are bound with the function to a functor using boost::bind(). This functor is stored inside context_object class (derived from context_base (both in sub-namespace detail).
Ummm, okay, which begs the question: where does this context_object object live?
I couldn't find it in the documentation, and whatever the answer, seems like something that should be mentioned.
Hmm - it is an implementation detail. Should I document it in the 'Rational' section?
Well, it seems like you need to dynamically allocate *something*, and whether this is through operator new, the stack allocator (probably preferable...?), or something else, I think it should be documented [...]
Given that void pointers are used to communicate between contexts, I almost would've expected the context constructor to simply take a void (*)( void*) argument and context::start to take a void* argument which would be passed to the function pointer. Can you elaborate on the rationale for the low-level interface for communicating between contexts while still providing a relatively high level interface for specifying the context function?
With context::start you enter the context-function. The arguemnts for the context-fn are taken by the ctor of context. context::resume and context::suspend have a void* arg because it is used to transfer data between context jumps.
Okay. In other words: you can type-erase the context function quite easily, since you use the pimpl idiom anyway. On the other hand, type-erasing the data transferred between contexts would necessitate, e.g., boost::any, and hence a potentially unnecessary dynamic allocation and (maybe) dynamic dispatch. context ctx;
void f( int i) { ... char * x = "abc"; void * vp = ctx.suspend( x); double * d = static_cast< double >( vp);
Of course, you mean double* here, right? (If this is an actual example from the library, be sure to change it!)
... } ctx = context( f, 1); void * vp = ctx.start(); char * x = static_cast< char * >( vp); double d = 2.01; ctx.resume( & d);
the cotnext-fn 'f()' is passed to contexts ctor with the integer arg '1'. ctx.start() starts the context == enter f() with argument i == 1. ctx.suspend() suspends execution of context-fn f() so we return from ctx.start() and transfers x == 'abc' as return arg of ctx.start(). ctx.resume() re-enters context-fn f() and the return value of ctx.suspend() is a pointer to the double withvalue 2.01.
The documentation for the stack allocator concept doesn't completely define the semantics of the size parameter of stack_allocator::allocate/deallocate. I presume it is the stack size in bytes, but that should be explicit in the documentation. And I presume allocate is only required to return a pointer to *at least* size bytes of free memory.
yes
Also, you don't mention alignment requirements; what are they?
the stack will be aligned automatically by boost.context - the user has not to worry about it.
I'm not sure who you mean by the "user". Just to be clear: I'm asking, what are the alignment requirements of the void* returned by allocate within the stack allocator concept? For those who want to create their own custom stack allocator.
I'm somewhat surprised default_stacksize() always returns 256*1024. I
think it might be safer to make this implementation-defined, but some minimum size (as with everything else in C++...).
which value should be choosen for default_stacksize()? The operating system doesn't define it (but minimum/maximum stacksize is defined) - it is a heuristic value.
Let's be clear, I'm not challenging your choice of the default stack size; of course it's heuristic. What I'm pointing out is what I perceive to be an over-specification of the interface. I don't think it's a good idea to guarantee (as much as one is able to, anyway) a one-size-fits-all default stack size for all present and future platforms for all time and across all dimensions and parallel universes. For the same reason that C++ doesn't define an int as a specific number of bits. It seems preferable to make it "implementation defined" and, perhaps, note its value on some common platforms. Does that make sense? That aside, can you provide some rationale for the specific choice of 256 kB? It seems rather...arbitrary, that's all. Indeed, I would've expected the default stack size to be somehow related to the platform word size (sizeof( void* ), maybe?).
Just a nitpick on member variable names.
boost_fcontext_stack::ss_base/ss_limit: what does "ss" mean?
names base and limit are inspired by MS Windows Fiber API.
Hmmm...okay. That doesn't exactly answer my question, though :/ - Jeff