I have only received one review (privately) so far, and hope that
those participating in the discussions about the library (as well as
any other interested boosters) would be willing to contribute a few
more - a summary of your final thoughts with a vote would be greatly
appreciated.

If you need a little extra time to finish out the discussions or write
the review, please let me know as we can extend the review period for
a few days if necessary.

Please feel free to submit your reviews to the list, or privately to
me.  As a reminder, here are some questions you may want to answer in
your review:

    * What is your evaluation of the design?
    * What is your evaluation of the implementation?
    * What is your evaluation of the documentation?
    * What is your evaluation of the potential usefulness of the library?
    * Did you try to use the library?  With what compiler?  Did you
have any problems?
    * How much effort did you put into your evaluation? A glance? A
quick reading? In-depth study?
    * Are you knowledgeable about the problem domain?

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.

Thanks again to those that have been participating in the discussions
and/or have already submitted a review.

Regards,

* 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.

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

Regards,
--
Joel de Guzman
http://www.boost-consulting.com
http://spirit.sf.net

I'm not sure it really suits the name 'switch_', as I'd expect something
syntactically different (something like Joel sketched out in his review,
and with some argument forwarding), however, I think it's an important
building block that should be kept as simple as possible.

I think it should probably be a part of MPL. It would fit perfectly next
to 'mpl::for_each' -- 'mpl::for_petrified_constant' or so :).

As pointed out in other posts I think the default default case behavior
should be changed not to throw but to use the default constructor if no
default case function object is specified.
A default case function object returning 'void' should be assumed (and
'assert'ed) not to return in a context where a (non-void) result is
expected (implementation hint: the built-in comma operator allows void
arguments and an overloaded comma operator doesn't).

As also pointed out and discussed in other posts I'm very much for
having the result type specified explicitly (as opposed to deducing it
from the function object).

If "fallthrough cases" are going to be implemented it should be done in
another template (or should it even be a full-blown Duff loop?).

Another variant of the template taking min/max instead of a sequence
might be a good idea, as it can make things compile faster in many
typical use cases (well, that would be half-way a design thing).

As mentioned before, the reference could be more detailed at places,
regarding the equality with MPL constants and the exact types passed to
the function objects (even if overloading operator() is uncommon - it
might be occasionally useful to deduce a non-type constant from it).

Regards,
Tobias

Here's an acid test for the API -- try to implement my suggested
syntax on top of the "simple" API. You'll soon realize that you
can't --without having to write the same amount of PP expansions all
over again. It's not a suitable building block, like say,
mpl::for_each.

Regards,
--
Joel de Guzman
http://www.boost-consulting.com
http://spirit.sf.net

Joel de Guzman <joel <at> boost-consulting.com> writes:

In Christ,
Steven Watanabe

Joel de Guzman <joel <at> boost-consulting.com> writes:

template<class Variant, class Visitor>
struct visitor_applier {
    typedef typename Visitor::result_type result_type;
    template<class N>
    result_type operator()(N) {
        visitor(
            variant.get<typename mpl::at<typename variant::types, N>::type>());
    }
    Visitor& visitor;
    Variant& variant;

switch_<result_type>(n, *make_tuple(*
    case<1>(f1),
    case<2>(f2),
    ...
))

It's possible to implement this in terms of the interface I
chose and vise versa.  Either one is inconvenient for some
tasks.  I optimized the interface for the uses that I understood
best.

Thanks a lot for your review, Joel.

In Christ,
Steven Watanabe

Tobias Schwinger <tschwinger <at> isonews2.com> writes:

Thanks!

In Christ,
Steven Watanabe

What would be the point in phoenix::switch_ if it wasn't lazy?

As Steven pointed out, overloading operator() (which is roughly what you
propose - just using an uglier syntax) is a rare use case. Usually we'll
want to feed the index to some metaprogramming machinery and instantiate
several high-speed dispatched control paths.
This utility is about generating the machine code of 'switch'. It does
not necessarily have to mimic it's syntax!

o  Use fusion::unfused or wire up some operators to build a fusion::map
of constant / function object pairs, and

o  create a function object from that map that looks up the appropriate
function object in the map and calls it (if we want it to be non-lazily
evaluated) or returns it (otherwise) and is fed to 'switch_'.

Please note that the reverse requires indeed to repeat the PP code.
Also note that it would also work to implement the variant visitor.

Also also note that the 'switch_' name is obviously confusing :-).

Regards,
Tobias