[Boost-users] Review: Switch library

Here's my review:

* What is your evaluation of the design?

It's a very simple design. Very straightforward. I'm not quite sure about the interface though: * The specification of cases is a bit cumbersome. In the common case, they are both specified in the switch_ call and in the individual function call overloads supplied by the client (e.g.): 1) switch_ >(n, f) 2) void F::operator()(mpl::int_<1>) void F::operator()(mpl::int_<2>) void F::operator()(mpl::int_<3>) Redundant. * I'd prefer the result type to be user-specified like in Boost.Bind instead of hard-wiring it to F::result_type. * The client needs to provide a specialized function object for the case detection. There's no way to use plain functions or even Boost.Function. I've implemented switch_ many times now. Here are some links: http://tinyurl.com/28e8y2 and http://tinyurl.com/ypmgob Having said all that, my preferred syntax is: switch_(n, case_<1>(f1), case_<2>(f2), case_<3>(f3), default_(fd) ); Key points: * The return type is specified in the call, where it should be. * You specify the cases only once, and not using a cumbersome MPL range or some such. * You supply N functions, not one humongous function with all the cases in. In many cases, you don't have a control over the functions, especially if you are writing a library. Think modularity. * Plain function pointers and Boost.Functions are fine. * The syntax is very "idiomatic" and as close to the native switch statement. * Fall-through is a possibility with additional syntax and thought. Example, by default we fall-through, but with: case_<3>(f3, break_) we don't. * Multiple cases are possible. E.g: case_<3, 5>(f3) * One disadvantage is that the type of the case is always an int. Perhaps we can specify that ala MPL: case_(f3) Another solution is to detect the type of the supplied (/n/ in the example) and cast the cases to its type. That way, it should be ok to have unsigned too. We only deal with the largest static integer (long or long long) and cast to the supplied int type. ... I think that would work, but I'm just thinking out loud as I write this, so I'd like to hear other people's thoughts.

* What is your evaluation of the implementation?

Haven't had time to check. But coming from Steven, It should be A+.

* What is your evaluation of the documentation?

Too terse. More examples needed. I think other folks have commented about this, so, I'll stop. I'm more concerned about the design.

* What is your evaluation of the potential usefulness of the library?

Very useful!

* Did you try to use the library? With what compiler? Did you have any problems?

No and no.

* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

Ehm, just read the docs and studied the design.

* Are you knowledgeable about the problem domain?

Very.

And finally, every review should answer this question:

* Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.

Not at the moment. I think we need a more thorough discussion on alternative interfaces. We also need to discuss the issues that were raised in the review. I'm eager to hear Steven's replies. He seem to be a bit too quiet? I'm really tempted to say "yes" and let Steven address the concerns raised (including mine). I'm very confident in Steven's abilities. He's one of those who still gives me the "oooh" feeling with his code. And, I really *NEED* such a switch utility now and not later. So, please take this as a soft "no" vote for now. I encourage Steven to get more involved in the discussion and consider all the issues raised. As soon as these matters are ironed out, fire up another review ASAP. Regards, -- Joel de Guzman http://www.boost-consulting.com http://spirit.sf.net