Re: [boost] Formal Review: Indexed Set
Hello Jeff, thanks for reviewing the indexed_set! ----- Mensaje original ----- De: Jeff Garland <jeff@crystalclearsoftware.com> 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
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
participants (2)
-
Jeff Garland -
JOAQUIN LOPEZ MU?Z