Boost logo

Boost :

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

Pavel has started an independent thread to treat these points.

> The docs mention
> 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
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

Boost list run by bdawes at, gregod at, cpdaniel at, john at