From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2007-03-11 10:33:27
Hi to all,
First of all, sorry for being late with this review summary for Bimap.
As I mentioned in my review result post, reviews have been very
positive, but there are still a few design decisions that weren't fully
ironed in the review threads. Let's enumerate the reviews:
* * * * * * * * * * * * * * * * * * * * * * * * * * *
[start of summary]
Alisdair Meredith did a positive review and kindly requested support for
Borland compilers. This would require changes in Boost.MultiIndex (on
which Bimap is based).
Fernando Cacciola suggested improving the introduction "What is a bimap"
with a picture explaining the nature of a bimap.
Matias has already created a new picture based on Fernando's
suggestions. The only point Matias does not agree is that the "above"
view is now written in the bottom of the picture.
Fernando did some suggestions to clarify some paragraphs in the
documentation (e.g the Tutorial) and Matias agreed to modify some of
them. The picture of the "standard mapping framework" section was also
mentioned as a possible point of improvement.
Jeff noted that the namespace "bimap" of the documentation should be
boost::bimap (- note of the review manager -: the namespace issue is
discussed later in this summary) and suggested a top level
boost/bimap.hpp header. He also suggested changing the example from the
tutorial to use "bimap::value_type" instead of "bimap::relation". Matias
considered that the one minute tutorial should only contain the sides
views and that set_of_relations should be introduced later.
Jeff commented that const iterators should be used in the "A simple
example" code and considered essential adding an example showing the
behavior when inserting a key that is already in the container. He
considered that the tutorial should have more examples, and in general,
more documentation explaining the many features bimap offers.
std::map compatibility: Jeff proposed making bimap compatible with
std::map, but Matias replied that the left/right views are the right
answer for std::map compatibility and that he wanted to maintain bimap
as a new view of the container (a set of relations). (- note of the
review manager -: this issue is discussed in another later)
Proposed a longer description of what a bimap is and considered that
"tagging" description should be improved and proposed changing the title
of the section.
Since the complexity notation is quite different from the usual one,
John proposed linking complexity notation with the explanation of terms.
John detected some errors while compiling the examples related to the
namespace name issue.
(- Note of the review manager -: This thread was the longest thread of
the review: operator(),at() and first-second/left-right, were the key
topics. The left-right issue will be discussed later on this summary post.)
Thorsten considered operator complicated. Matias explained that that
current implementation is the most conservative one. Thorsten would
maintain only the non-mutable versions or just remove the function.
Matias agreed to remove the non-mutable version but still had doubts.
Thorsten proposed adding at(). Matias agreed to add only the non-mutable
version and commented that the mutable version should only be available
if the other view is not associative to avoid breaking container invariants.
Several member additions were discussed:
--> "replace" overloads: replace(it, key, value), replace(it, key) and
--> insert( const key&, const key2& ). This would need Boost.Multiindex
changes. Joaquin commented that this would need a bit of work and that
there were some allocator interface limitations that he would need to
avoid. Placement new was suggested. The issue remains open.
--> Insertions from convertible values. The goal is to reduce the number
of called copy-constructors.
--> Construction from keys without constructing a relation lead to a
discussion about how Boost.MI could offer the needed infrastructure to
achieve a delayed construction facility. Joaquin put this issue in his
"to study" list.
Apart from this, Thorsten suggested improvements to the documentation
text in several sections.
Tony (I couldn't get the surname, sorry)
---- "One Minute Tutorial" needs two additional lines: #include <boost/bimap/bimap.hpp> using namespace bimap; Tony considered "relation" too generic and proposed "couple"/"duo"/"bipair" alternatives. Matias replied that he preferred staying with "relation" first-second and left-right thread ---------------------------------- Matias started a new thread explaining the reasons behind the map<X,Y> interface for side views and set<relation> for the "above" view. John Maddock suggested a ".relation" member to access to the set<relation> facilities. Thorsten agreed. Matias considered that there are reasons to maintain set<relation> functions as bimap members (for example "size()"). This issue remains open with the following key points: --> Should bimap offer "relation" views? --> In that case, should this view be accessed using a ".relation" (or other name) member or should we consider bimap as the view? --> Should "relation" (or another name) contain left-right or first-second members? I hope this issues will be solved soon with mailing list discussions. new name for the container -------------------------- According to Boost rules, the namespace should be called boost::bimaps. Thorsten proposed injecting boost::bimaps::bimap into the boost namespace. A <boost/bimap.hpp> header was also considered useful. [end of summary] * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * I've tried to collect the most important points of the review process, but obviously I might have missed some points or misunderstood some positions/conclusions. If that's the case, please correct me! Regards, Ion
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk