Re: [boost] Formal Review: Indexed Set

From: Brian McNamara (lorgon_at_[hidden])
Date: 2004-03-20 19:00:34

On Fri, Mar 19, 2004 at 11:53:08PM +0100, Pavel Vozenilek wrote:
> The review of Indexed Set library, written by
> Joaquín M López Muñoz starts today (March 20)
> and runs for 10 days.
> 
> Latest version (9.4) of the library is available on:
> http://groups.yahoo.com/group/boost/files/indexed_set.zip
> (328 kB)

I spent an hour or two today looking at the docs and trying out the
library on some very simple examples.  Here are my comments.

   What is your evaluation of the design?

Looks good; I have wished for a container along these lines in the past,
and the interface that indexed_set presents seems satisfactory.

   What is your evaluation of the implementation?

I didn't look at it.

   What is your evaluation of the documentation?

I read the "tutorial" and only glanced at the other sections.  The
documentation is quite good.  A few quick comments:

Some of the code examples in the tutorial have lines longer than 80
chars and they don't render well in my browser.  Breaking the long
lines into multiple lines will easily fix this.

At one point in the tutorial it says "nth_indexed_type" instead of
"nth_index_type".  The same (extra "ed") bug is repeated again
shortly after the first one.

I had to manually
   #include "boost/indexed_set/sequenced_index.hpp"
in addition to
   #include "boost/indexed_set.hpp"
in order to get "sequenced" to work.  Not sure if this is a bug in
the docs or a bug in the library itself; either it should be included
automatically, or else the docs should mention including it manually.

The docs mention
   #define BOOST_INDEXED_SET_ENABLE_INVARIANT_CHECKING
for both "safe mode" and "invariant checking mode".  I did not look
into this enough to decide if there is (or is supposed to be) a
different separate flag to set for "safe mode".

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

At least once in the past I have wanted this functionality myself, and
also once others have asked me if I knew a good library for doing this.
So I think it will be useful.

   Did you try to use the library?  

Yes.

   With what compiler?  

g++-3.1.1

   Did you have any problems?

Of course.  :)

I uncovered a compiler bug, which was annoying, but irrelevant to the
review.  (Had to say "es.template get<by_name>()" rather than
"es.get<by_name>()", even though it was in a non-dependent context.)

At one point, when using tags, I did something like

   struct by_seq {};
   ... sequenced<by_seq> ...  // oops, should be  sequenced<tag<by_seq> >

and got a compiler error.  (BTW, the docs are not totally clear here;
they should say that you need tag<...>.)  Fortunately it was a
BOOST_STATIC_ASSERT failure, and inspecting the failing line of code
inside the library:

   ...
   template <typename TagList=tag<> >
   struct sequenced
   {
     BOOST_STATIC_ASSERT(detail::is_tag<TagList>::value);   // this line
   ...

gave me enough of a clue to figure out the problem on my own, so that
was very happy.

I noted with some dismay that duplicate tags are not detected.  That
is, you are allowed to say

   struct by_id {};
   struct by_name {};
   struct by_foo {};
   typedef indexed_set<
     employee,
     index_list<
       unique<tag<by_id,by_foo>,identity<employee> >,               
       non_unique<tag<by_name,by_foo>,
                  member<employee,std::string,&employee::name> >
     > 
   > employee_set;

where I have named both indices "by_foo".  I would prefer it if the
library detected this as a BOOST_STATIC_ASSERT-able error.

Other than that, things worked pretty smoothly.

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

I spent an hour or two today looking at the docs and trying out the
library on some very simple examples.

   Are you knowledgeable about the problem domain? 

Not really; I have used std::list and std::set, but never really used a
real multiply-indexed data structure or tried to implement one on my
own.

   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.

Yes.

-- 
-Brian McNamara (lorgon_at_[hidden])