Re: [boost] Formal Review: Indexed Set

From: JOAQUIN LOPEZ MU?Z (joaquin_at_[hidden])
Date: 2004-03-21 08:38:23

Hello Brian, thank you for reviewing indexed_set!

----- Mensaje original -----
De: Brian McNamara <lorgon_at_[hidden]>
Fecha: Domingo, Marzo 21, 2004 1:00 am
Asunto: Re: [boost] Formal Review: Indexed Set

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

Thank you, I'll fix these.

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

Well, this is not actually a bug, buy an issue I would
like people to give their opinion about, along with other
naming and organizatinal issues. If you feel in the mood,
please take a look at the review notes at

http://groups.yahoo.com/group/boost/files/indexed_set_review_notes.html

Pavel has started an independent thread to treat these points.

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

It's a bug in the docs. There exists a separate
BOOST_INDEXED_SET_ENABLE_SAFE_MODE macro. Thank you for
spotting this out.

[...]
> 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<...>.) [...]

Well, the requirement is stated at the referece, though
it won't hurt to say it more explicitly in the tutorial.
I'll try to improve this.

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

Currently, the approach is that the first index with
a given tag is picked (the reference states this explicitly).
I'm not convinced the approach you suggest is more
convenient: maybe eliminating such duplicate tags in a
metaprogramming environmnt (imagine the indexed_set is
instantiated by an automatic framework) is not that simple.
A static *warning* would be ideal here :)

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

great ;)

Joaquín M López Muñoz
Telefónica, Investigación y Desarrollo