Hello Brian, thank you for reviewing indexed_set! ----- Mensaje original ----- De: Brian McNamara <lorgon@cc.gatech.edu> 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