[Boost-users] Review: Switch library

From: Joel de Guzman (joel_at_[hidden])
Date: 2008-01-09 06:18:09

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_<mpl::vector_c<int, 1, 2, 3> >(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_<return_type>(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_<char, 'x'>(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