Re: [boost] Formal Review: Indexed Set

From: JOAQUIN LOPEZ MU?Z (joaquin_at_[hidden])
Date: 2004-03-28 06:56:02

Hello Jeff, thanks for reviewing the indexed_set!

----- Mensaje original -----
De: Jeff Garland <jeff_at_[hidden]>
Fecha: Domingo, Marzo 28, 2004 5:21 am
Asunto: [boost] Formal Review: Indexed Set

> Let me start by saying thanks to Joaquín for the contribution of 
> this library.

You are welcome. The experience has been a lot of fun
to me.

[...]
> 
> * Do you think the library should be accepted as a Boost library?  
> Yes.

great :) thanx!

> 
> *********
> Now for some questions / comments:
> 
> 1) Dead code in index_node.hpp?
> 
> I don't think the ':' is legal below -- I assume this is dead code?
> 
> template<typename Super> struct 
> index_node_trampoline:index_node_impl{};
> template<typename Super>
> struct index_node:Super,index_node_trampoline<Super>
> ...
> 

This is not dead code. index_node_treampoline:index_node_impl
is short for index_node_treampoline:public index_node_impl.
My coding style is admiteddly a little too compact.

> 2) bidirectional_map
> 
> My first interest in this library started when it was just a bi-
> directionalmap.  Now it has become something much broader and more 
> powerful.  Curiously,
> the bi-directional map part seems to have taken a back seat -- 
> being pushed
> into the examples as an 'exercise for the reader'.
> 
> I think that this is unfortunate.  I like to see bidirectional_map 
> as an
> official part of the library.  A few reasons for this: 
> 
>  a) bidirectional_map will be documented up front as a part of 
> the library.  
>  b) For me it is my current primary use of this library -- even 
> though it is
> now so flexible I'll have to rethink how I use containers in designs.
>  c) Complex code is pushed on the user.  For example, the code 
> below  is from
> examples/bimap.cpp
> 
> template<typename FromType,typename ToType>
> struct bidirectional_map
> {
>  typedef std::pair<FromType,ToType> value_type;
> 
> #if defined(BOOST_NO_POINTER_TO_MEMBER_TEMPLATE_PARAMETERS) ||\
>    defined(BOOST_MSVC)&&(BOOST_MSVC<1300) ||\
>   
> defined(BOOST_INTEL_CXX_VERSION)&&defined(_MSC_VER)&&(BOOST_INTEL_CXX_VERSION<=700)
> 
> ....
> 
> It is obvious from this that there are some portability issues.  
> Putting this
> sort of detail on library users is too mcch to expect from users 
> in my opinion.
> 
> But it is only a suggestion since it took me all of 5 minutes to 
> pull out the
> core of the example to a header file.

I don't think the bidirectional map I provide in the
examples section is good enough for officialization
(for one, it lacks operator[]). I only wrote it as
an example of use which I think will be common enough.
IMHO a much more user-friendly bidirectional map could be
developed based on indexed_set, but we are talking
probably of a separate library. Also, when I first
introduced bimap to Boost a year ago, people expressed
interest in an extensible approach, which bimap's
rigid interface certainly did not provide.

As for the macro stuff, you probably shouldn't be
concerned if the compiler you work with is reasonably
conformant.

> 
> 3) Serialization support
> In future directions you say  "Once Robert Ramey's serialization 
> library gets
> accepted into Boost..."
> 
> I'd love to see Joaquín at least attempt this soon -- it would 
> serve as an
> excellent review of serialization which is upcoming shortly :-)

Good idea, if time permits I'll engage into the review and
try to sketch some serialization support for indexed_set.

> 
> 4) Should the SGI/HP copyrights be included in the source files 
> which are
> based off the stl_tree.h implementation?  

I don't know. The original pieces of code (with lots
of changes) are scattered across some five headers
or so. Basically it is the rb-tree algorithms that
I have retained. Maybe some of the licensing wizards here
at the list can offer advice on this issue.

> 
> 5) Naming
> 
> For the namespace/library name I suggest 
> boost::multiindex_containers. 
> 
> Rationale:
> -- The library provides several stl-like container types (set, 
> list, map) --
> hence the plural 'containers'.
> -- The containers all have multiple indexes -- hence multiindex.
> 

Maybe boost::container::multi_index would please
more people: it is as descriptive as your proposal,
but (1) it is into boost::container and (2) avoids
the abhorred "ii".

Best,

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