Re: [boost] Formal Review: Indexed Set

From: Jeff Garland (jeff_at_[hidden])
Date: 2004-03-28 11:05:28

On Sun, 28 Mar 2004 13:56:02 +0200, JOAQUIN LOPEZ MU?Z wrote

> > 1) Dead code in index_node.hpp?
>...
> 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.

Ah, yes...thx.  

As an aside, I've had pretty good luck with a formatter called artistic style.
  
http://astyle.sourceforge.net/

Although, I'll admit I haven't tried it on heavy template code.  That said, it
would clean up that spacing in a jiffy :-)

> > 2) bidirectional_map
> > 
> > My first interest in this library started when it was just a bi-
> > directionalmap. 
>....
> > I like to see bidirectional_map as an
> > official part of the library.  A few reasons for this: 
>...
> 
> 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

Well, sure.  I personally don't care about operator[] 
because I don't even like using it with std::map.  So 
for me it is ok to just document that it doesn't exist.

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

Agreed -- what more would you add to the example?

>but we are talking probably of a separate library. 

I don't see why.  It would be a specifically targeted 
multi-index container built on the flexible core.  To me,
your library is now a family of collection types.  Adding 
targeted easy to use collections in the library makes perfect 
sense.

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

Sure, but the flexibility almost always comes at the
cost of user ease of use.  No question about it in
this case.  I can't say to another programmer: go read
the bimap doc page and use it do this...

I now have to say.  Go look at the examples.  Hack out
the macro stuff (if we are using a conformant compiler),
create a bimap.hpp, write a little test suite so we can
be sure it works like we expect, write up some docs to
explain how this works for other users.  Don't forget 
that operator[] doesn't work, etc...

But I'm starting to whine now.  I'm just pointing out
that you have lost something for some of us...

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

Sure, but my point is that as a user I'm suddenly exposed
to the details.  On any given day I might or might not
be working with a conformant compiler. If bimap.hpp is
part of the library I don't look at the header I just
write my code.

> > 
> > 3) Serialization support
>...
> > I'd love to see Joaquín at least attempt this soon -- it would 
>...
> Good idea, if time permits I'll engage into the review and
> try to sketch some serialization support for indexed_set.

Great, thx.

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

In reading the license in the docs, I don't think there is a problem with
compatibility, but I believe it would be safer to also include this notation
in the affected source -- even though the licenses sort of implies it isn't
necessary.  
<usual disclaimer>
I am not a lawyer.  The above is not legal advice.
</usual disclaimer>

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

boost::container::multi_index works for the namespace.  But to be clear, I'm
expecting you are going to rename the entire library to Multi-Indexed
Containers, right?  Also, are you suggesting that I'm going to see
BOOST_ROOT/libs/container/multi_index and
BOOST_ROOT/boost/container/multi_index?  This is what was just done with the
string algorithms library (BOOST_ROOT/libs/algorithm/string/).

While this structure makes perfect sense to me, it creates an inconsistency
with existing container libraries like array and multi_array which are not
under the .../container subtree.  Personally, I'd like to see array,
multi-array, and dynamic_bitset get moved under a .../container tree -- but it
is alot of work. 

The other option would be to have paths like libs/multi_index_containers/, but
 now it doesn't match the namespace...

Jeff