[boost] Variant Review: variant iterators

From: Jeff Garland (jeff_at_[hidden])
Date: 2003-03-12 06:17:15

All -

I'm posting this review (with permission of the author) that
was not posted on the list during the review.  This review was
considered as part of the library acceptance.  Anyway, the review
may be of interest to other variant users as it is quite detailed 
and implements a novel use of variants...

Jeff

-----Original Message-----
Sent: Sunday, March 02, 2003 11:00 AM

Dear boosters,

  just my two pence on the variant library.

  To evaluate the variant library, I implemented a variant_iterator
  type with the help of boost::iterator_adaptor and boost::variant.
  The main aspect here is that all iterator operators, such as dereference,
  increment, etc., must be implemented such that they work for the variant.

  For simplicity, I restricted myself to *two* iterator types; the general
  case with n types might be (much!) more complicated.
  For details, see the attached file. Environment:
  Win XP, MSVC++ 7.0, STLPort 4.5.3, Boost 1.29.0 + everything from Boost sandbox

  Issues I came across in no particular order:

  + I regard it a bad practice to put all the files either straight
    into the boost directory, or into boost/detail/variant.

    Since variant is no "simple library implemented within the library header",
    boost/variant and boost/variant/detail would be conforming to the Boost
    directory structure guidelines.

    For the same reason, I think everything should be put into namespace
    boost::variant.

    Further, I'd like to include everything in one line:

     #include "boost/variant/variant.hpp"

  + The equality operator is missing.

    I guess it occurs pretty often that someone has to test two
    variant<T0,...,Tn>'s for equality, i.e. that they are of same type
    and same value, and I think this should be fixed before being
    submitted to Boost.

  + So I had to write operator==() myself, and stumbled over another issue.

    Two variants are equal if the have same type and same value, so a
    hand-written version might look like:

      bool operator()( const boost::variant<T0,T1>& v0,
                       const boost::variant<T0,T1>& v1 ) const
      {
        if( v0.which() != v1.which() ) return false; // same type?
        try {
          switch( v0.which() ) {
             case 0: // same value?
               return( boost::extract<T0>(v0)() == boost::extract<T0>(v1)() );
             // etc...
             }
          }
        catch( boost::bad_extract& exc )
          { /* etc... */ }
      }

    This implementation, however, throwed bad_extract's in any case.
    Changing the header to pass-by-value instead pass-by-reference, i.e.

      bool operator()( boost::variant<T0,T1> v0, boost::variant<T0,T1> v1 ) const

     solved the problem. Weird.

  + Even in cases when the two iterator types are different,
    derived types might be equal; for instance,
      std::vector<int>::iterator is different from std::list<int>::iterator,
    but
      std::vector<int>::value_type and std::list<int>::value_type are the same

    Since variant<T0,T1> requires T0 and T1 to be different, I got

      boost::variant<...>::preprocessor_list_initializer::initialize( void*,.. ):
      member function already defined or declared with ...

    a) Is it possible to issue an error message that is better understandable?

    b) Since I expect this issue to appear quite frequently, my wish would be
       to have a helper construct at hand which, for any type list T0,...,Tn,
       calculates a typle list S0,...,Sk were all duplicate types are removed.
       If all T0,...,Tn are the same, this might even boil down to the common
       type T. I had to write such a variant_helper for n=2, and I'm aware
       that the general case is more complicated.
       OK, that's only a wish, it might as well be delayed for any
       Boost 1.3x release.

  + Various typos in the docs; from the tutorial:

    "the closet possible type" ... "Naturally, If there" ...
    "no convesion is applied"

    A spell checker might be helpful.

  Conclusion:

  Provided that the equality operator for variant is added to the library,
  I vote "yes": variant should be accepted into Boost.

  After all, it seems stable enough (and definitely useful!), so I'll
  immediatetely start using it.

  - Roland