[boost] [context] mini-review comments

Subject: [boost] [context] mini-review comments
From: Jeffrey Lee Hellrung, Jr. (jeffrey.hellrung_at_[hidden])
Date: 2012-01-12 21:07:43

[Late, I know]

[I'm going off
http://ok73.ok.funpic.de/boost/libs/context/doc/html/index.html]

[Apologies if these are just repeating previous comments!]

How do you store the function and function arguments passed to the context
constructor? Are they bundled into memory allocated by the stack allocator?
I couldn't find it in the documentation, and whatever the answer, seems
like something that should be mentioned.

"The maximum number of arguments of *context-function* is defined by
BOOST_CONTEXT_ARITY (default is 10) and might be changed."
Hopefully you mean, more specifically, that BOOST_CONTEXT_ARITY might be
*increased*. Also, consider using variadic templates if available?

Great examples! Would it be possible to also show the expected output, just
so the reader can verify that he or she understands what should be
happening?

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?

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. Also, you don't mention alignment requirements; what are they?

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++...). Also, strictly speaking, the
documentation is incorrect. It reads "Defines a default stack size of 256
kB", but defalt_stacksize() doesn't define anything, it just returns a
number. And this description is inconsistent with the remaining function
descriptions. I would do: "Returns the default stack size, which is
implementation-defined but at least 256 kB."

In "Low level API (boost_fcontext_t)", s/stated/started/. Also, what's the
magic number 262144? Oh, I see, 262144 = 256 * 1024...maybe the latter
expression would be more intuitive? Again, I can guess what the output of
the example would be, but would be helpful to have the expected output
handy.

Just a nitpick on member variable names.
boost_fcontext_stack::ss_base/ss_limit: what does "ss" mean?
boost_fcontext::fc_stack/fc_link: I presume the prefix "fc" is short for
"fcontext", in which case, isn't it redundant? Normally I wouldn't be picky
about member variable names but these are precisely the documented API.

In "Rationale", you give "[o]ther low level APIs and their caveats"; I'm
guessing these are, specifically, alternatives to boost_fcontext_t but are
*deficient* in some respect. It might clear things up if you make that
explicit, and further group "setjump()/longjmp()", "ucontext_t", and
"Windows fibers" into a common heading.

Overall, I think the documentation and interface are good. I didn't look at
the implementation, but I did see that someone else did, so...I'm not too
worried about a correct implementation. As long as there's a full suite of
tests. I also didn't try to use the library, but now I'll have to find an
excuse :)

These are all relatively minor comments, and although I hope Oliver will
address them, it won't hold up my approval of Boost.Context.

- Jeff